Skip to content

Commit 162d587

Browse files
authored
fix(sampling): ensure all tags in a sampling rule are matched (#14097)
Trace sampling rules now require all specified tags to be present for a match, instead of ignoring missing tags. Additionally, all glob patterns (e.g., *, ?, [ ]) now work with numeric tags, including decimals. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
1 parent 7fb968d commit 162d587

5 files changed

+192
-46
lines changed

ddtrace/_trace/sampling_rule.py

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -111,46 +111,38 @@ def matches(self, span: Span) -> bool:
111111
:returns: Whether this span matches or not
112112
:rtype: :obj:`bool`
113113
"""
114-
tags_match = self.tags_match(span)
115-
return tags_match and self._matches((span.service, span.name, span.resource))
114+
return self.tags_match(span) and self._matches((span.service, span.name, span.resource))
116115

117116
def tags_match(self, span: Span) -> bool:
118-
tag_match = True
119-
if self._tag_value_matchers:
120-
tag_match = self.check_tags(span.get_tags(), span.get_metrics())
121-
return tag_match
117+
if not self._tag_value_matchers:
118+
return True
122119

123-
def check_tags(self, meta, metrics):
124-
if meta is None and metrics is None:
120+
meta = span._meta or {}
121+
metrics = span._metrics or {}
122+
if not meta and not metrics:
125123
return False
126124

127-
tag_match = False
128-
for tag_key in self._tag_value_matchers.keys():
129-
value = meta.get(tag_key)
130-
# it's because we're not checking metrics first before continuing
125+
for tag_key, pattern in self._tag_value_matchers.items():
126+
value = meta.get(tag_key, metrics.get(tag_key))
131127
if value is None:
132-
value = metrics.get(tag_key)
133-
if value is None:
134-
continue
135-
# Floats: Matching floating point values with a non-zero decimal part is not supported.
136-
# For floating point values with a non-zero decimal part, any all * pattern always returns true.
137-
# Other patterns always return false.
138-
if isinstance(value, float):
139-
if not value.is_integer():
140-
if all(c == "*" for c in self._tag_value_matchers[tag_key].pattern):
141-
tag_match = True
142-
continue
143-
else:
144-
return False
145-
else:
146-
value = int(value)
147-
148-
tag_match = self._tag_value_matchers[tag_key].match(str(value))
149-
# if we don't match with all specified tags for a rule, it's not a match
150-
if tag_match is False:
128+
# If the tag is not present, we failed the match
129+
# (Metrics and meta do not support the value None)
130+
return False
131+
132+
if isinstance(value, float):
133+
# Floats: Convert floats that represent integers to int for matching. This is because
134+
# SamplingRules only support integers for matfching or glob patterns.
135+
if value.is_integer():
136+
value = int(value)
137+
elif set(pattern.pattern) - {"?", "*"}:
138+
# Only match floats to patterns that only contain wildcards (ex: * or ?*)
139+
# This is because we do not want to match floats to patterns like `23.*`.
140+
return False
141+
142+
if not pattern.match(str(value)):
151143
return False
152144

153-
return tag_match
145+
return True
154146

155147
def sample(self, span):
156148
"""
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
fixes:
3+
- |
4+
sampling: Trace sampling rules now require all specified tags to be present for a match, instead of ignoring missing tags.
5+
Additionally, all glob pattern that do not contain digits (e.g., *, ?, [ ]) now work with numeric tags, including decimals.

tests/integration/test_sampling.py

Lines changed: 46 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -243,10 +243,33 @@ def test_extended_sampling_tags_and_name_glob():
243243

244244

245245
@pytest.mark.snapshot()
246-
@pytest.mark.subprocess(env={"DD_TRACE_SAMPLING_RULES": json.dumps([{"sample_rate": 0, "tags": {"tag": "2*"}}])})
246+
@pytest.mark.subprocess(
247+
env={
248+
"DD_TRACE_SAMPLING_RULES": json.dumps(
249+
[{"sample_rate": 0, "service": "mycoolservice", "tags": {"tag1": "monkey", "tag2": "banana"}}]
250+
)
251+
}
252+
)
253+
def test_extended_sampling_tags_partial_match():
254+
"""
255+
For a span to match a sampling rule it must contain all the tags listed in the rule.
256+
Partial matches are not allowed.
257+
"""
258+
from ddtrace.trace import tracer
259+
260+
with tracer.trace(name="should_send", service="mycoolservice") as span:
261+
span.set_tag("tag1", "monkey")
262+
263+
with tracer.trace(name="should_not_send", service="mycoolservice") as span:
264+
span.set_tag("tag1", "monkey")
265+
span.set_tag("tag2", "banana")
266+
267+
268+
@pytest.mark.snapshot()
269+
@pytest.mark.subprocess(env={"DD_TRACE_SAMPLING_RULES": json.dumps([{"sample_rate": 0, "tags": {"tag1": "2*"}}])})
247270
def test_extended_sampling_float_special_case_do_not_match():
248-
"""A float with a non-zero decimal and a tag with a non-* pattern
249-
# should not match the rule, and should therefore be kept
271+
"""A float with a non-zero decimal and a tag with a pattern
272+
that contains a digit should not match the rule, and should therefore be kept.
250273
"""
251274
from ddtrace.trace import tracer
252275

@@ -255,16 +278,32 @@ def test_extended_sampling_float_special_case_do_not_match():
255278

256279

257280
@pytest.mark.snapshot()
258-
@pytest.mark.subprocess(env={"DD_TRACE_SAMPLING_RULES": json.dumps([{"sample_rate": 0, "tags": {"tag": "*"}}])})
281+
@pytest.mark.subprocess(
282+
env={
283+
"DD_TRACE_SAMPLING_RULES": json.dumps(
284+
[
285+
{"sample_rate": 0, "tags": {"tag": "*"}},
286+
{"sample_rate": 0, "tags": {"tag2": "?*"}},
287+
{"sample_rate": 0, "tags": {"tag3": "**"}},
288+
]
289+
)
290+
}
291+
)
259292
def test_extended_sampling_float_special_case_match_star():
260-
"""A float with a non-zero decimal and a tag with a * pattern
261-
# should match the rule, and should therefore should be dropped
293+
"""A float with a non-zero decimal and a tag with a glob pattern that does
294+
not contain a digit should match the rule and should therefore should be dropped
262295
"""
263296
from ddtrace.trace import tracer
264297

265-
with tracer.trace(name="should_send") as span:
298+
with tracer.trace(name="should_not_send") as span:
266299
span.set_tag("tag", 20.1)
267300

301+
with tracer.trace(name="should_not_send2") as span:
302+
span.set_tag("tag2", 22.2)
303+
304+
with tracer.trace(name="should_not_send3") as span:
305+
span.set_tag("tag3", 3333333.33333)
306+
268307

269308
@pytest.mark.subprocess()
270309
def test_rate_limiter_on_spans(tracer):
Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,81 @@
11
[[
22
{
3-
"name": "should_send",
3+
"name": "should_not_send",
44
"service": "ddtrace_subprocess_dir",
5-
"resource": "should_send",
5+
"resource": "should_not_send",
66
"trace_id": 0,
77
"span_id": 1,
88
"parent_id": 0,
99
"type": "",
1010
"error": 0,
1111
"meta": {
1212
"_dd.p.dm": "-3",
13-
"_dd.p.tid": "6600aa0700000000",
13+
"_dd.p.tid": "68824df100000000",
1414
"language": "python",
15-
"runtime-id": "8eef084f89104256b5f82b95c235139c"
15+
"runtime-id": "79453d6c5643438bbdf8e2c1d9548877"
1616
},
1717
"metrics": {
1818
"_dd.rule_psr": 0.0,
1919
"_dd.top_level": 1,
2020
"_dd.tracer_kr": 1.0,
2121
"_sampling_priority_v1": -1,
22-
"process_id": 53744,
22+
"process_id": 65903,
2323
"tag": 20.1
2424
},
25-
"duration": 25000,
26-
"start": 1711319559844282297
25+
"duration": 22000,
26+
"start": 1753370097140066000
27+
}],
28+
[
29+
{
30+
"name": "should_not_send2",
31+
"service": "ddtrace_subprocess_dir",
32+
"resource": "should_not_send2",
33+
"trace_id": 1,
34+
"span_id": 1,
35+
"parent_id": 0,
36+
"type": "",
37+
"error": 0,
38+
"meta": {
39+
"_dd.p.dm": "-3",
40+
"_dd.p.tid": "68824df100000000",
41+
"language": "python",
42+
"runtime-id": "79453d6c5643438bbdf8e2c1d9548877"
43+
},
44+
"metrics": {
45+
"_dd.rule_psr": 0.0,
46+
"_dd.top_level": 1,
47+
"_dd.tracer_kr": 1.0,
48+
"_sampling_priority_v1": -1,
49+
"process_id": 65903,
50+
"tag2": 22.2
51+
},
52+
"duration": 14000,
53+
"start": 1753370097140825000
54+
}],
55+
[
56+
{
57+
"name": "should_not_send3",
58+
"service": "ddtrace_subprocess_dir",
59+
"resource": "should_not_send3",
60+
"trace_id": 2,
61+
"span_id": 1,
62+
"parent_id": 0,
63+
"type": "",
64+
"error": 0,
65+
"meta": {
66+
"_dd.p.dm": "-3",
67+
"_dd.p.tid": "68824df100000000",
68+
"language": "python",
69+
"runtime-id": "79453d6c5643438bbdf8e2c1d9548877"
70+
},
71+
"metrics": {
72+
"_dd.rule_psr": 0.0,
73+
"_dd.top_level": 1,
74+
"_dd.tracer_kr": 1.0,
75+
"_sampling_priority_v1": -1,
76+
"process_id": 65903,
77+
"tag3": 3333333.33333
78+
},
79+
"duration": 9000,
80+
"start": 1753370097140891000
2781
}]]
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
[[
2+
{
3+
"name": "should_send",
4+
"service": "mycoolservice",
5+
"resource": "should_send",
6+
"trace_id": 0,
7+
"span_id": 1,
8+
"parent_id": 0,
9+
"type": "",
10+
"error": 0,
11+
"meta": {
12+
"_dd.base_service": "ddtrace_subprocess_dir",
13+
"_dd.p.dm": "-0",
14+
"_dd.p.tid": "6881b20100000000",
15+
"language": "python",
16+
"runtime-id": "e5827348f1274e43a58dc88d9dc65596",
17+
"tag1": "monkey"
18+
},
19+
"metrics": {
20+
"_dd.top_level": 1,
21+
"_dd.tracer_kr": 1.0,
22+
"_sampling_priority_v1": 1,
23+
"process_id": 51177
24+
},
25+
"duration": 21000,
26+
"start": 1753330177405084000
27+
}],
28+
[
29+
{
30+
"name": "should_not_send",
31+
"service": "mycoolservice",
32+
"resource": "should_not_send",
33+
"trace_id": 1,
34+
"span_id": 1,
35+
"parent_id": 0,
36+
"type": "",
37+
"error": 0,
38+
"meta": {
39+
"_dd.base_service": "ddtrace_subprocess_dir",
40+
"_dd.p.dm": "-3",
41+
"_dd.p.tid": "6881b20100000000",
42+
"language": "python",
43+
"runtime-id": "e5827348f1274e43a58dc88d9dc65596",
44+
"tag1": "monkey",
45+
"tag2": "banana"
46+
},
47+
"metrics": {
48+
"_dd.rule_psr": 0.0,
49+
"_dd.top_level": 1,
50+
"_dd.tracer_kr": 1.0,
51+
"_sampling_priority_v1": -1,
52+
"process_id": 51177
53+
},
54+
"duration": 15000,
55+
"start": 1753330177405259000
56+
}]]

0 commit comments

Comments
 (0)