-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[processor/tailsampling] Move from OpenCensus to OTel API #33426
[processor/tailsampling] Move from OpenCensus to OTel API #33426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jpkrohling, this needs a rebase
4148c3c
to
7a39ac8
Compare
Rebased! |
7a39ac8
to
363cf18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks ok, are there any test to validate the emitted telemetry (other than your manual tests?
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
2d3eac9
to
0339654
Compare
Good idea, I added some tests now to show how the telemetry is created for each metric. |
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jpkrohling!
This PR migrates the internal telemetry for the tail sampling processor to OpenTelemetry. All metric names were kept, as well as their labels and other properties, such as bucket boundaries, so that this change should not cause disruptions to current users of this component.
One thing that I did notice was that, previously, metrics for the probabilistic sampler were included as if that component was used. However, the probabilistic sampling policy was used:
otelcol_processor_probabilistic_sampler_count_traces_sampled
. This PR corrects that. Users relying on that should rely onotelcol_processor_tail_sampling_global_count_traces_sampled
instead.Here's how the metrics looked like before this change (I'm copying all metrics, in case I'm missing something):
And here are the new metrics:
They were generated by making a telemetrygen call for 100 traces to a Collector with this configuration:
Closes #31581
Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de