Consistent reservoir sampling span processor #352
Conversation
| OtelTraceState otelTraceState = OtelTraceState.parse(otelTraceStateString); | ||
| int pval; | ||
| int rval; | ||
| long priority = randomGenerator.nextLong(); |
There was a problem hiding this comment.
I understand that among spans with the same r-value, the priority decides which spans will be removed from the reservoir. Giving each span a new random value may break trace completeness to a larger degree than it is necessary.
To alleviate this problem, the priority might be made dependent on trace-id, for example by combining a hash of trace-id and a (less significant) random number. Since there are no guarantees about quality of trace-id in general, I do not advocate to do it by default, but it would be nice to refactor the code so it would be easy to do for the users who really want it.
There was a problem hiding this comment.
Thanks, this improvement could be added in a later PR without breaking anything. As it would require choosing a hash function, which is a difficult decision by itself, much more discussion would be needed. (Ideally, it should be a fast, high-quality function for which there are compatible implementations in all relevant programming languages.) I think as a first step, this PR should be merged without this improvement.
There was a problem hiding this comment.
What I meant by refactoring was something like calculating the priority using an overridable method with trace-id as a parameter. It would remain unused in the default implementation.
But I agree with you that it can be done later.
|
@PeterF778 thanks for reviewing! would you be interested in being a "component owner" for the consistent sampling component? (see #174) this is a tricky component and we only have a single component owner currently which slows down reviews and merges for it. |
|
Yes, I'd be interested! Thanks for the offer!
Peter
…________________________________
From: Trask Stalnaker ***@***.***>
Sent: Monday, June 13, 2022 1:37 PM
To: open-telemetry/opentelemetry-java-contrib ***@***.***>
Cc: Peter Findeisen (pfindeis) ***@***.***>; Mention ***@***.***>
Subject: Re: [open-telemetry/opentelemetry-java-contrib] Consistent reservoir sampling span processor (PR #352)
@PeterF778<https://github.com/PeterF778> thanks for reviewing! would you be interested in being a "component owner" for the consistent sampling component? (see #174<#174>) this is a tricky component and we only have a single component owner currently which slows down reviews and merges for it.
—
Reply to this email directly, view it on GitHub<#352 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AVXBUCLA7MLXWWKUSNCRTKTVO6LZBANCNFSM5XEQH4VA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
great! I'll send a PR to add you |
|
Closing and re-opening to see if reviewers get updated |
It didn't work, I think because the branch is not up-to-date with main (which has the new component owners updates). This is not a problem though. @PeterF778 if you can finish reviewing and/or approve then I will do a quick check and merge. thx! |
...rc/main/java/io/opentelemetry/contrib/samplers/ConsistentReservoirSamplingSpanProcessor.java
Outdated
Show resolved
Hide resolved
Description:
Implementation of a consistent reservoir sampling span processor