Skip to content
This repository was archived by the owner on Apr 2, 2024. It is now read-only.

Implement ingested_spans_count in telemetry data. #1155

Merged
merged 2 commits into from
Feb 16, 2022
Merged

Implement ingested_spans_count in telemetry data. #1155

merged 2 commits into from
Feb 16, 2022

Conversation

harkishen
Copy link
Member

@harkishen harkishen commented Feb 16, 2022 •
edited
Loading

Signed-off-by: Harkishen-Singh [email protected]

Description

This PR adds ingested spans count to telemetry data collection. It does some refactoring to client as the trace ingestor needs to access telemetryEngine, which in turn also simplifies the NewClient()

Merge requirements

Please take into account the following non-code changes that you may need to make with your PR:

  • CHANGELOG entry for user-facing changes
  • Updated the relevant documentation

@harkishen harkishen self-assigned this Feb 16, 2022
@harkishen harkishen removed the request for review from a team February 16, 2022 10:02
@harkishen harkishen force-pushed the add_spans_count_in_telemetry branch from 5fe28b7 to 57f5d6f Compare February 16, 2022 10:06
paulfantom
paulfantom reviewed Feb 16, 2022
View reviewed changes
paulfantom
paulfantom suggested changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that for telemetry purposes metric name can be anything, but to keep everything consistent we should adhere to recent "consistent metric names" design document.

@harkishen harkishen force-pushed the add_spans_count_in_telemetry branch from 57f5d6f to d036197 Compare February 16, 2022 10:53
@harkishen
Copy link
Member Author

@paulfantom we should have a separate PR (after the release) that updates all telemetry metrics. This is why I didn't do in this PR.

paulfantom
paulfantom approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@paulfantom paulfantom left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Harkishen-Singh ack. I was confused and though this metric would be also exposed on prometheus /metrics endpoint, which is not the case.

LGTM on green

niksajakovljevic
niksajakovljevic reviewed Feb 16, 2022
View reviewed changes
pkg/pgclient/client.go Outdated
)
telemetryEngine, err = telemetry.NewEngine(dbConn, PromscaleID, queryable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend keeping telemetry engine creation outside of Client (that reads and writes). Imho those are different concerns. It would also make it easier to test Client as we can just inject telemetry engine...

Copy link
Member Author

@harkishen harkishen Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But telemetry engine needs queryable and the client needs telemetry (for passing down to IngestTraces). So if I keep it outside, like in runner.go, it will be a egg-chicken problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, let's leave this since it's blocking the release. I have created an issue #1156 which I will solve after the release is done.

@harkishen harkishen force-pushed the add_spans_count_in_telemetry branch 3 times, most recently from fa9f5e7 to 37eea31 Compare February 16, 2022 12:13
antekresic
antekresic approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@antekresic antekresic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good with the exception to what Niksa mentioned. Since that will be handled later on, I'm OK with merging this.

@harkishen harkishen force-pushed the add_spans_count_in_telemetry branch from 37eea31 to c0b130b Compare February 16, 2022 13:28
@harkishen
Fix alter column to be in 0.10.0-dev and not in 0.8.0-dev
d3484a0 
Signed-off-by: Harkishen-Singh <[email protected]>

This commit fixes the wrongly added
2-alter_promscale_instance_information_column.sql to 0.8.0 when the next
release scheduled was 0.10, meaning the changes to schema wont be
applied. This was a bug.

This commit shifts the alter column file to where it should be, i.e.,
0.10.0/1-alter_promscale_instance_information_column.sql
which means, the existing users when update to 0.10 will have the alter
column.
@harkishen harkishen force-pushed the add_spans_count_in_telemetry branch from c0b130b to d3484a0 Compare February 16, 2022 13:45
@harkishen harkishen enabled auto-merge (rebase) February 16, 2022 13:46
@harkishen harkishen merged commit 9391f8c into timescale:master Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jgpruitt jgpruitt Awaiting requested review from jgpruitt

3 more reviewers

@niksajakovljevic niksajakovljevic niksajakovljevic left review comments

@antekresic antekresic antekresic approved these changes

@paulfantom paulfantom paulfantom approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@harkishen harkishen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@harkishen @antekresic @paulfantom @niksajakovljevic