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

tracer: limit the scope of setting the priority when auto dropped #2157

Conversation

katiehockman
Copy link
Contributor

What does this PR do?

Only change the priority of the trace, not the individual span, if it's dropped upon creation. This is a follow up of #2067

Motivation

We only need this because a sampling decision must be made before a chunk is partially flushed. Even though this will create 1 additional allocation, and a ~5% execution time degradation from the current code on main, it will only affect a single span in a trace. Once the decision has been made for the first span in the trace, then that decision will just be used for the remaining spans, so this code won't be reached. Typical use cases wouldn't notice any change in performance. It's also worth noting that this allocation likely would have been created anyway, just later down the line, because the trace priority will be finalized before flushing. Also note that this only creates one more allocation for this benchmark per trace, even though the benchmark makes it look like it's per span, because there is only a single pointer to a float for the entire trace.

pkg: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer
                  │   old.txt   │              new4.txt              │
                  │   sec/op    │   sec/op     vs base               │
TracerAddSpans-10   1.633µ ± 2%   1.710µ ± 2%  +4.72% (p=0.000 n=10)

                  │   old.txt    │              new4.txt               │
                  │     B/op     │     B/op      vs base               │
TracerAddSpans-10   2.124Ki ± 0%   2.132Ki ± 0%  +0.37% (p=0.000 n=10)

                  │  old.txt   │             new4.txt              │
                  │ allocs/op  │ allocs/op   vs base               │
TracerAddSpans-10   22.00 ± 0%   23.00 ± 0%  +4.55% (p=0.000 n=10)

Note that this is still better than the original PR, which had a ~10% performance time hit. Though this still probably would have been negligible for users:

pkg: gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer
                  │   old.txt   │              new.txt               │
                  │   sec/op    │   sec/op     vs base               │
TracerAddSpans-10   1.633µ ± 2%   1.788µ ± 4%  +9.49% (p=0.000 n=10)

                  │   old.txt    │               new.txt               │
                  │     B/op     │     B/op      vs base               │
TracerAddSpans-10   2.124Ki ± 0%   2.132Ki ± 0%  +0.37% (p=0.000 n=10)

                  │  old.txt   │              new.txt              │
                  │ allocs/op  │ allocs/op   vs base               │
TracerAddSpans-10   22.00 ± 0%   23.00 ± 0%  +4.55% (p=0.000 n=10)

Describe how to test/QA your changes

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

@katiehockman katiehockman marked this pull request as ready for review July 28, 2023 20:41
@katiehockman katiehockman requested a review from a team July 28, 2023 20:41
@katiehockman
Copy link
Contributor Author

I'm going to hold off on merging this until @ajgajg1134 is able to look at this, since he made the original change. I want to make sure I'm not missing something here, and the performance impact is ~negligible anyway, across an entire trace.

@@ -624,7 +623,7 @@ func (t *tracer) sample(span *span) {
sampler := t.config.sampler
if !sampler.Sample(span) {
span.context.trace.drop()
span.setSamplingPriority(ext.PriorityAutoReject, samplernames.RuleRate)
span.context.trace.setSamplingPriority(ext.PriorityAutoReject, SamplingRuleSpan)
Copy link
Contributor

Choose a reason for hiding this comment

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

SamplingRuleSpan is the wrong type here this is the value which gets used in _dd.p.dm to mark why we dropped this (although it probably doesn't actually matter here since this trace will be dropped, it's still useful for any possible metrics calculated from this value even if the trace itself isn't ingested)

Suggested change
span.context.trace.setSamplingPriority(ext.PriorityAutoReject, SamplingRuleSpan)
span.context.trace.setSamplingPriority(ext.PriorityAutoReject, samplernames.RuleRate)

@ajgajg1134
Copy link
Contributor

This was merged to main in #2176

@ajgajg1134 ajgajg1134 closed this Aug 17, 2023
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.

3 participants