Skip to content
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

Add new tail sampling processor policy: latency #3750

Merged
merged 4 commits into from
Jun 14, 2021

Conversation

yvrhdn
Copy link
Contributor

@yvrhdn yvrhdn commented Jun 10, 2021

Description:

This adds a new policy to the tail sampling processor sampling traces based upon the duration of a span. Adding this policy allows you to sample all traces of which the duration exceeds a given threshold.

It's my first time contributing to Open-Telemetry, let me know if this fits with the other code 😊

Link to tracking Issue:

Testing:

Added unit tests similar to the other policies.

Documentation:

I have updated the README.

@yvrhdn yvrhdn requested a review from jpkrohling as a code owner June 10, 2021 13:20
@yvrhdn yvrhdn requested a review from a team June 10, 2021 13:20
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 10, 2021

CLA Signed

The committers are authorized under a signed CLA.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I like this, and the code looks sane. But allow me to make one suggestion: how about taking the trace duration, instead of the span? Or at least, have an option for users to make the decision based on the trace or on spans.

@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 10, 2021

But allow me to make one suggestion: how about taking the trace duration, instead of the span? Or at least, have an option for users to make the decision based on the trace or on spans.

Agree, this is a great suggestion. I'll add this tomorrow 🙂

@jpkrohling
Copy link
Member

Alright, let me know when you need a new review. I'm out next week, though, so you might need to ask someone else to review this if you can't wait for me to return.

@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 11, 2021

Alright, I have switched the implementation to check the entire duration of a trace 👍 I think this use case will be way more common.

I wasn't sure how to calculate the total duration of a trace, I've implemented it by searching the min/max timestamps of all spans. Does this seem alright?

Copy link
Member

@jpkrohling jpkrohling 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! There's a whole world of possible algorithms to determine the real duration of the trace, but this simple "wall clock" approach is correct most of the time for most of the users.

I would just make sure to document this decision in the readme, so that users understand what "trace duration" means for our purposes.

processor/tailsamplingprocessor/README.md Outdated Show resolved Hide resolved
processor/tailsamplingprocessor/sampling/latency.go Outdated Show resolved Hide resolved
Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>
@yvrhdn
Copy link
Contributor Author

yvrhdn commented Jun 11, 2021

I would just make sure to document this decision in the readme, so that users understand what "trace duration" means for our purposes.

Thanks! Your suggestions have been added 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants