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

spanprofiler: do less work on unsampled traces #528

Merged
merged 1 commit into from
May 31, 2024
Merged

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented May 30, 2024

What this PR does:

Skip the work to examine the span and decorate it, if the whole trace is not sampled hence the data is going nowhere.
This is 4% of all garbage in one production Mimir installation I looked at, maybe 2% of all CPU.

BenchmarkTracer/unsampled-28             2866868               405.6 ns/op           979 B/op         10 allocs/op
BenchmarkTracer/sampled-28               1000000              1024 ns/op            2346 B/op         25 allocs/op

Checklist

  • Tests updated
  • CHANGELOG.md updated

Skip the work to examine the span and decorate it, if the whole trace
is not sampled hence the data is going nowhere.

Signed-off-by: Bryan Boreham <[email protected]>
@bboreham bboreham merged commit 5141842 into main May 31, 2024
6 checks passed
@bboreham bboreham deleted the skip-unsampled branch May 31, 2024 09:58
@kolesnikovae
Copy link
Contributor

I probably should have made this more clear, but there is a reason for collecting sampled traces: span_name attribution (aggregated span profiles):

} else {
// Even if the trace has not been sampled, we still need to keep track
// of samples that belong to the span (all spans with the given name).
ctx = pprof.WithLabels(parentCtx, pprof.Labels(
spanNameLabelName, operationName))
}

On the other hand, I suppose that probabilistic trace sampling might not impact the results of profiling sampling (structurally and proportionally), so that even a fraction of span profiles forms a trustworthy picture.

I guess it might make sense to add an option that controls the behaviour

@bboreham
Copy link
Contributor Author

bboreham commented Jun 6, 2024

Sorry I missed that nuance.

It will affect the result: suppose a particular span name does 1x work in traces sampled 0.01 and 100x work in traces sampled 0.1.

But by this reasoning, the aggregate seems low-value. I need to know the cost in context.

@kolesnikovae
Copy link
Contributor

I'm going to add an option that controls this behavior. By default, sampled traces will be accounted for in span aggregate profiles. Would you mind?

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