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.

Sign up for GitHub

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

Jump to bottom

ddtrace/tracer: adding sampling by resource and tags #2448

Merged

dianashevchenko merged 11 commits into main from shevchenko/tag-resource-sampling

Jan 8, 2024

Contributor

dianashevchenko commented Dec 19, 2023

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

2 fields are added to the sampling rule - tags, resource
exactName and exactService are removed,, in favour of just Service and Name fields in the SamplingRule
changes in regexes in log_test.go relate to the removed exactName or exactService fields, thus regexes changed

Motivation

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
Add an appropriate team label so this PR gets put in the right place for the release notes.

For Datadog employees:

If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

dianashevchenko and others added 5 commits

November 30, 2023 16:59


          ddtrace/tracer: extending sampling rule struct with tags, resource an…

5ffad80

…d target_span (#2403)


          ddtrace/tracer: added DD_TRACE_SAMPLING_RULES_FILE env var (#2404)

66400e5


          ddtrace/tracer: account for resource, tags, and target_span when appl…

f45d036

…ying sampling rules (#2407)


          ddtrace/tracer: removed exactService and exactName from sampling rules (

e13e8d7

#2416)


          Merge branch 'main' into shevchenko/tag-resource-sampling

bb2601d

pr-commenter bot commented Dec 19, 2023 •

edited

Benchmarks

Benchmark execution time: 2024-01-08 14:59:07

Comparing candidate commit 5305d5f in PR branch shevchenko/tag-resource-sampling with baseline commit d28a899 in branch main.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 38 metrics, 2 unstable metrics.

scenario:BenchmarkTracerAddSpans-24

🟩 execution_time [-153.329ns; -92.671ns] or [-3.680%; -2.224%]

katiehockman reviewed

View reviewed changes

ddtrace/tracer/rules_sampler.go Outdated Show resolved Hide resolved

dianashevchenko added 2 commits

December 27, 2023 23:15


          ddtrace/tracer: removed target_span + trying to sample at the end (#2464

5d50d35


          ddtrace/tracer: remove target_span from sampling (#2449)

b7e0b88

github-actions bot reviewed

View reviewed changes

ddtrace/tracer/sampler_test.go

+              				_, _, _, stop := startTestTracer(t)
+              				defer stop()
+              				s, _ := StartSpanFromContext(context.Background(), "web.request",

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context

ddtrace/tracer/sampler_test.go

+              		tr, _, _, stop := startTestTracer(t)
+              		defer stop()
+              		originSpan, _ := StartSpanFromContext(context.Background(), "web.request",

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context

ddtrace/tracer/sampler_test.go

+              		w3cCtx, err := tr.Extract(headers)
+              		assert.Nil(t, err)
+              		w3cSpan, _ := StartSpanFromContext(context.Background(), "web.request", ChildOf(w3cCtx))

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context (typecheck)

ddtrace/tracer/sampler_test.go

+              				_, _, _, stop := startTestTracer(t)
+              				defer stop()
+              				s, _ := StartSpanFromContext(context.Background(), "web.request",

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context

ddtrace/tracer/sampler_test.go

+              		tr, _, _, stop := startTestTracer(t)
+              		defer stop()
+              		originSpan, _ := StartSpanFromContext(context.Background(), "web.request",

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context

ddtrace/tracer/sampler_test.go

+              		w3cCtx, err := tr.Extract(headers)
+              		assert.Nil(t, err)
+              		w3cSpan, _ := StartSpanFromContext(context.Background(), "web.request", ChildOf(w3cCtx))

github-actions bot Jan 3, 2024

🚫 [golangci] _{reported by reviewdog 🐶}
undefined: context) (typecheck)

dianashevchenko and others added 4 commits

January 3, 2024 16:47


          ddtrace/tracer: simplified SpanTagsResourceRule (#2470)

c89ec56


          lost import

83faea4


          ddtrace/tracer: locking sampling decision after context propagation (#…

55761ee

…2483)


          Merge branch 'main' into shevchenko/tag-resource-sampling

5305d5f

dianashevchenko marked this pull request as ready for review

January 8, 2024 15:00

dianashevchenko requested a review from a team as a code owner

January 8, 2024 15:00

darccio approved these changes

View reviewed changes

Contributor

darccio left a comment

LGTM

dianashevchenko merged commit a784a5f into main

153 checks passed

dianashevchenko deleted the shevchenko/tag-resource-sampling branch

January 8, 2024 15:37

darccio mentioned this pull request

ddtracer/tracer: update TestStartupLog/errors & TestLogSamplingRules to new sampling rules implementation in v2 #2492

Merged

8 tasks

knusbaum pushed a commit that referenced this pull request


          ddtracer/tracer: update TestStartupLog/errors & TestLogSamplingRules …

…to new sampling rules implementation in v2 (#2492)

Updates TestStartupLog/errors to handle the way that sampling rules work
in #2448, as in v2 it causes to return an error when creating the tracer.
The error is logged as before, but we don't need to run the tracer to
create it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment