ddtrace/tracer: extending sampling rule struct with tags, resource and target_span#2403
Conversation
BenchmarksBenchmark execution time: 2023-11-30 15:10:27 Comparing candidate commit f4d9f1d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 1 unstable metrics. |
ahmed-mez
left a comment
ahmed-mez
left a comment
There was a problem hiding this comment.
Looks good! Just added a few minor suggestions.
| Tags map[string]*regexp.Regexp | ||
|
|
||
| // TargetSpanRoot specifies whether sampling should be applied to the root spans only. | ||
| TargetSpanRoot bool |
There was a problem hiding this comment.
nit: perhaps TargetRootSpan or just TargetRoot ?
| }, { | ||
| }, | ||
| { | ||
| value: `[{"target_span": "root", "sample_rate": 1.0,"tags": {"host":"h-1234"}}]`, |
There was a problem hiding this comment.
Can we cover any and "" as possible values of target_span in the tests?
There was a problem hiding this comment.
This test only check validity of the unmarshalling, will add the checks when i add sampling logic itself
There was a problem hiding this comment.
This code block
var targetRoot bool
switch strings.ToLower(v.TargetSpan) {
case "", "root":
targetRoot = true
case "any":
targetRoot = false
default:
errs = append(errs, fmt.Sprintf(`at index %d: "target_span" value is not expected, must be in ["any", "root", ""]`, i))
continue
}is in the unmarshalling logic that is being tested, the unit tests of unmarshalSamplingRules should cover it imho.
Current coverage misses any:
| ruleN: 1, | ||
| }, | ||
| { | ||
| value: `[{"target_span": "root", "sample_rate": 1.0, "tags": {"host":"h-1234"}}]`, |
There was a problem hiding this comment.
should be any not root 😅
There was a problem hiding this comment.
will fix now, in the different branch though
3 participants
What does this PR do?
This PR extends the sampling rules to include resource, tags, and target_span. The sampling logic will be added in the subsequent PRs. This PR only modifies the SamplingRule struct, json marshalling and unmarshalling.
Motivation
As part of the Extending sampling efforts to include resource, tags, and target_span fields in the sampling logic.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!