ddtrace/tracer: account for resource, tags, and target_span when applying sampling rules#2407
Conversation
BenchmarksBenchmark execution time: 2023-12-01 15:01:58 Comparing candidate commit 50bcd69 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics. |
knusbaum
left a comment
knusbaum
left a comment
There was a problem hiding this comment.
A couple comments.
I think there's some stuff I'm not familiar with in shevchenko/tag-resource-sampling such as TagsResourceRule that it would be helpful to have a PR up for as well.
|
|
||
| // match returns true when the span's details match all the expected values in the rule. | ||
| func (sr *SamplingRule) match(s *span) bool { | ||
| if sr.TargetRoot && s.root().SpanID != s.SpanID { |
There was a problem hiding this comment.
How likely is s.root() to return nil, or for s to be nil?
There was a problem hiding this comment.
Span can be newly created with all the necessary / fields / context filled out, but never nil. Subsequently, root wouldn't be nil. The match function is called only within sampling functionality on a trace / chunk, and thus span is never empty.
| v, ok := s.Meta[k] | ||
| if !ok || !regex.MatchString(v) { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like this should match both Meta and Metrics.
It's not specified in the RFC how numbers should be represented. Maybe talk with the authors about how this should work.
2 participants
What does this PR do?
This PR changes the sampling rule matching logic. Tests are updated accordingly.
Additionally, updated the MarshalJSON method of the SamplingRule.
What to remember when reviewing
"target_span":"any",where it was applicableMotivation
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!