Repository navigation
sampling weight & tracestate propagation - #1384
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
|
eyalkoren
left a comment
There was a problem hiding this comment.
Regarding your comment- "as an optimization, we might avoid tracestate parsing and transmit as-is if it does not contains es= string", I think it is more than a nice-to-have, we need to optimize already now.
Let's try to break it:
- starting a child transaction
- if parent passed a
tracestate- if the
tracestateheader containses=- only parse from its start position to the next,. No need to split and iterate over all of it. - else (
tracestateheader does not containes=) - we are required to split for vendor entries because the spec does not allow more than 32 entries. However, in most cases there will be less than 32, so it should be enough to append our own to the end of it.
- if the
- else (parent did not pass a
tracestateheader) - create a single-entrytracestatewith our value.
- if parent passed a
- starting a root transaction - create a single-entry
tracestatewith our value.
This is already a big optimisation, as it means we will only have to split the header if our agent is intermixed with other vendors, which is probably not the common case.
Furthermore, whenever we add our tracestate entry, since the sample rate comes from the configuration, we can easily create a readymade tracestate entry and register a listener on the transaction_sample_rate config option to replace it when it is changed.
|
We now have
|
Co-authored-by: eyalkoren <41850454+eyalkoren@users.noreply.github.com>
| higherBound = (long) (Long.MAX_VALUE * samplingRate); | ||
| lowerBound = -higherBound; | ||
| this.sampleRate = samplingRate; | ||
| traceStateHeader = TraceState.buildHeaderString(samplingRate); |
There was a problem hiding this comment.
I don't think we need this dependency in TraceState - enough to be able to get sampleRateAsString here and use the the StringBuilder in TraceState to append to the header when required. This means both no allocation and no dependency.
Sorry for nagging, but conceptually - a sampler shouldn't be aware of tracestate
eyalkoren
left a comment
There was a problem hiding this comment.
Let's not keep this hanging just because of #1384 (comment) - I'd like to avoid this dependency, but if it complicates stuff, don't bother.
I think if TextTracestateAppender#join() would accept List<CharSequence> instead of List<String> that would make things simpler as you can combine Strings and StringBuilders in the tracestate list.
eyalkoren
left a comment
There was a problem hiding this comment.
👍 Thanks for your patience 😊
Codecov Report
@@ Coverage Diff @@
## master #1384 +/- ##
=========================================
Coverage 60.85% 60.85%
Complexity 3087 3087
=========================================
Files 388 388
Lines 17952 17952
Branches 2507 2507
=========================================
Hits 10925 10925
Misses 6313 6313
Partials 714 714 Continue to review full report at Codecov.
|
What does this PR do?
Fixes #1293
Fixes #1385
Implementation notes / questions
Sampler#getSampleRate()is relevant as there might be non-probabilistic samplers which can't provide any meaningful value. For root transactions, we could simply get the value from configuration instead.tracestateheadertracestateparsing and transmit as-is if it does not containses=string,Checklist
Added an API method or config option? Document in which version this will be introduced