ddtrace/tracer: adding support for single span matching #1357

dianashevchenko · 2022-06-27T13:24:12Z

Motivation: Single Span Ingestion Control RFC

Recap:

glob matcher regex matches full strings, only '?' and '*' are treated as metacharacters
2 mandatory single span tags(mechanism and rule rate) should be set on matched spans, max_per_second as well if available
span rules loaded from the env variable take precedence over a file with rules and a warning is emitted

…ion isn't keep

ajgajg1134

Looks pretty good! Mostly just some nit-picky comments here and there :)

ddtrace/ext/tags.go

ddtrace/tracer/option.go

ddtrace/tracer/sampler.go

ddtrace/tracer/single_sampler.go

ddtrace/tracer/tracer_test.go

katiehockman

Thanks, Diana! I didn't get a chance to go very deep into the review, and it's very possible that some of my comments won't be very clear, so I'm happy to pair with you on any of it this week if you'd like. That may make it easier for me to explain what I'm going for here.

Comments about the PR description:
"a warning is emitted" can you make it clear that it is only emitted if both the rules file and rules env var are provided, thus dropped the file?

ddtrace/ext/tags.go

ddtrace/tracer/option.go

ddtrace/tracer/sampler.go

katiehockman · 2022-06-29T22:13:19Z

ddtrace/tracer/sampler.go

+// DD_SPAN_SAMPLING_RULES_FILE environment variables.
+func spanSamplingRulesFromEnv() ([]SamplingRule, error) {
+	errs := []string{}
+	rules, err := processSamplingRulesFromEnv([]byte(os.Getenv("DD_SPAN_SAMPLING_RULES")), SingleSpanSamplingType)


Do we need to do the same check here as we do for trace sampling rules? e.g.

rulesFromEnv := os.Getenv("DD_SPAN_SAMPLING_RULES") if rulesFromEnv == "" { return nil, nil } return processSamplingRulesFromEnv([]byte(rulesFromEnv), SingleSpanSamplingType)

ddtrace/tracer/sampler.go

gbbr

Left a few comments, but there are a lot already. Once they get addressed, I can look again.

ddtrace/tracer/sampler.go

gbbr · 2022-06-30T08:10:18Z

ddtrace/ext/tags.go

+	SpanSamplingMechanism = "_dd.span_sampling.mechanism"
+
+	// SingleSpanSamplingMechanism is a constant reserved to indicate that a span was kept on account of a span sampling rule.
+	SingleSpanSamplingMechanism = 8


Are any of these constants meant to be used by users? Why are they public API?

that is good point, movedt to tracer/sampler

Let's only resolve the comment once the change is committed please.

ddtrace/tracer/option.go

ddtrace/tracer/util.go

ddtrace/tracer/spancontext.go

gbbr · 2022-06-30T08:27:01Z

ddtrace/tracer/tracer.go

+	// rules for applying a sampling rate to spans that match the designated service
+	// or operation name. Such spans are sampled if trace sampling decision is 'drop' and
+	// may be sent separately
+	singleSpanRulesSampling *singleSpanRulesSampler


There are too many samplers. We should have just one: rulesSampler and that one should have two methods (instead of apply), called:

sampleTrace

sampleSpan

// sampleTraces reports whether an entire trace should be kept based solely on the given span, // which is usually the first in the chunk.

and

// sampleSpan reports whether this individual span should be kept.

I understand that the RFC was called "single span sampling" but it may be much clearer to say "individual span sampling" and it's fine to do that especially in internal methods IMHO.

It might actually be more tedious to combine them into one. I'll leave this open for now

ddtrace/tracer/sampler.go

gbbr · 2022-06-30T08:31:25Z

ddtrace/tracer/option.go

+
+	// spanSamplingRules contains user-defined rules determine the sampling rate to apply
+	// to single spans, regardless of the trace sampling decision.
+	spanSamplingRules []SamplingRule


How are these defined and why are they here? There is no option (With*) which sets them.

They are supplied through env variable, like I've mentioned above I think of adding analogous option, without renaming old one (WithSamplingRules)

Can we just have WithSamplingRules which defines all of them? Both for spans and for traces. You already added a Type field to SamplingRule, that should be enough. You can pick them apart into two sets inside the rulesSampler implementation.

…pler to a new struct; moved traceSampler code to a separate file; updated sampler to include rules from WithSamplingRules option where both types of rules can be set;

…/DataDog/dd-trace-go into shevchenko/single-span-ingestion

dianashevchenko · 2022-07-21T15:21:16Z

Can't reproduce test failure with 2k runs

ajgajg1134

This is looking great! 👏
just some small comments but overall approved once those changes are looked at / made!!

ddtrace/tracer/rules_sampler.go

ddtrace/tracer/tracer_test.go

…op level tag anymore

knusbaum · 2022-07-07T17:12:59Z

ddtrace/tracer/log_test.go

@@ -55,7 +55,7 @@ func TestStartupLog(t *testing.T) {
 		tp.Reset()
 		logStartup(tracer)
 		assert.Len(tp.Lines(), 2)
-		assert.Regexp(`Datadog Tracer v[0-9]+\.[0-9]+\.[0-9]+(-rc\.[0-9]+)? INFO: DATADOG TRACER CONFIGURATION {"date":"[^"]*","os_name":"[^"]*","os_version":"[^"]*","version":"[^"]*","lang":"Go","lang_version":"[^"]*","env":"configuredEnv","service":"configured.service","agent_url":"http://localhost:9/v0.4/traces","agent_error":"Post .*","debug":true,"analytics_enabled":true,"sample_rate":"0\.123000","sample_rate_limit":"100","sampling_rules":\[{"service":"mysql","name":"","sample_rate":0\.75}\],"sampling_rules_error":"","service_mappings":{"initial_service":"new_service"},"tags":{"runtime-id":"[^"]*","tag":"value","tag2":"NaN"},"runtime_metrics_enabled":true,"health_metrics_enabled":true,"profiler_code_hotspots_enabled":((false)|(true)),"profiler_endpoints_enabled":((false)|(true)),"dd_version":"2.3.4","architecture":"[^"]*","global_service":"configured.service","lambda_mode":"false","appsec":((true)|(false)),"agent_features":{"DropP0s":false,"Stats":false,"StatsdPort":0}}`, tp.Lines()[1])
+		assert.Regexp(`Datadog Tracer v[0-9]+\.[0-9]+\.[0-9]+(-rc\.[0-9]+)? INFO: DATADOG TRACER CONFIGURATION {"date":"[^"]*","os_name":"[^"]*","os_version":"[^"]*","version":"[^"]*","lang":"Go","lang_version":"[^"]*","env":"configuredEnv","service":"configured.service","agent_url":"http://localhost:9/v0.4/traces","agent_error":"Post .*","debug":true,"analytics_enabled":true,"sample_rate":"0\.123000","sample_rate_limit":"100","sampling_rules":\[{"service":"mysql","name":"","sample_rate":0\.75,"type":0}\],"sampling_rules_error":"","service_mappings":{"initial_service":"new_service"},"tags":{"runtime-id":"[^"]*","tag":"value","tag2":"NaN"},"runtime_metrics_enabled":true,"health_metrics_enabled":true,"profiler_code_hotspots_enabled":((false)|(true)),"profiler_endpoints_enabled":((false)|(true)),"dd_version":"2.3.4","architecture":"[^"]*","global_service":"configured.service","lambda_mode":"false","appsec":((true)|(false)),"agent_features":{"DropP0s":false,"Stats":false,"StatsdPort":0}}`, tp.Lines()[1])


Since this is meant for debugging and confirming configuration, having a meaningful human-readable value for type would be better. Can we can convert that int to its enum/const name easily?

…/DataDog/dd-trace-go into shevchenko/single-span-ingestion

gbbr

This is looking good. I just have some comments regarding naming but overall LGTM!

gbbr · 2022-08-02T08:28:09Z

ddtrace/tracer/rules_sampler.go

+	return false
+}
+
+func (rs *singleSpanRulesSampler) applyRate(span *span, rule SamplingRule, rate float64, now time.Time) {


Not sure if you forgot this comment. If you feel strongly on having this additional method, it's ok, but I don't see the reason for it. If you keep it, maybe rename it like this:

Suggested change

func (rs *singleSpanRulesSampler) applyRate(span *span, rule SamplingRule, rate float64, now time.Time) {

func (rs *singleSpanRulesSampler) applyRule(span *span, rule SamplingRule, now time.Time) {

There is no point to have the rate argument since it's derived from rule.Rate

gbbr · 2022-08-02T08:30:37Z

ddtrace/tracer/tracer.go

-func (t *tracer) pushTrace(trace []*span) {
+// traceInfo contains span list and sampling decision of the trace
+// needed to run single span sampling.
+type traceInfo struct {


Generally when you find yourself using words like "data", "info", "process" or "util" in programming, I recommend questioning that choice and perhaps trying to find something better. These are words that generally apply in any situation and are not very specific. How about this?

Suggested change

type traceInfo struct {

type finishedTrace struct {

traceInfo sounds a bit as if it is information about a trace, whereas this is actually a finished trace with context about sampling decision.

gbbr · 2022-08-02T08:32:27Z

ddtrace/tracer/tracer.go

+	atomic.AddUint64(&t.partialTraces, 1)
+}
+
+func (t *tracer) pushTraceInfo(info *traceInfo) {


Despite you having changed the signature of this function by adding a new argument. I would not change the name. The "info" suffix is not needed imho.

gbbr · 2022-08-02T08:33:54Z

ddtrace/tracer/tracer.go

+
+// processTraceInfo runs single span sampling on the spans pushed to the worker thread.
+// Sampling spans on the worker thread keeps any associated latency off the application thread.
+func (t *tracer) processTraceInfo(info *traceInfo) {


Speaking of my previous comment, here we find both the word "process" and "info" as part of the same method. Double whammy 🙂 How about sampleFinishedTrace? This makes it clear that it is sampling which occurs at the end.

gbbr · 2022-08-02T08:34:07Z

ddtrace/tracer/tracer.go

+// needed to run single span sampling.
+type traceInfo struct {
+	spans            []*span
+	samplingDecision samplingDecision


Suggested change

samplingDecision samplingDecision

decision samplingDecision

Perhaps this is enough?

gbbr · 2022-08-02T08:36:02Z

ddtrace/tracer/tracer.go

+// traceInfo contains span list and sampling decision of the trace
+// needed to run single span sampling.


It's better to document something as what it is or what it stands for, rather than what its fields are and where it is used. Considering you follow my suggestions, I'd do something like:

// finishedTrace holds information about a trace that has finished, including its spans.

I would not mention that it's needed for sampling. When I read "this is needed for ..." it sounds a bit like it's an excuse for having it. As if it would not be here otherwise. Let's make it clear that it is useful instead.

gbbr · 2022-08-02T08:36:54Z

ddtrace/tracer/tracer.go

@@ -295,8 +302,11 @@ func (t *tracer) worker(tick <-chan time.Time) {
 			// before the final flush to ensure no traces are lost (see #526)
 			for {
 				select {
-				case trace := <-t.out:
-					t.traceWriter.add(trace)
+				case info := <-t.out:


Suggested change

case info := <-t.out:

case trace := <-t.out:

Then, trace.decision and trace.spans makes complete sense.

gbbr · 2022-08-02T08:38:28Z

ddtrace/tracer/tracer.go

+	samplingDecision samplingDecision
+}
+
+// processTraceInfo runs single span sampling on the spans pushed to the worker thread.


This explanation is very technical and assumes that the reader has a lot of context. If possible (it's not always possible) we should try to document things in a more independent way, for example:

// sampleFinishedTrace applies single-span sampling to the provided trace, which is considered to be finished.

gbbr · 2022-08-02T08:39:02Z

ddtrace/tracer/tracer.go

+}
+
+// processTraceInfo runs single span sampling on the spans pushed to the worker thread.
+// Sampling spans on the worker thread keeps any associated latency off the application thread.


This also seems out of place and arguably not needed. A function's documentation should not assume that it is called from anywhere. It's just a function at this point.

gbbr

Also, lastly, I think we should update the docs and explain how to use single span sampling with environment variables and files. WDYT?

If you want to do that as a separate PR, that's fine!

gbbr

LGTM 👏🏻 🚀 🎉

dianashevchenko added 2 commits June 27, 2022 13:39

pkg/tracer: added glob matcher for span sampling

7044c89

pkg/tracer: added single span sampling rules & updated the matcher

d8fa045

dianashevchenko requested review from knusbaum, ajgajg1134, gbbr, katiehockman and a team June 27, 2022 13:24

dianashevchenko added this to the 1.39.0 milestone Jun 27, 2022

dianashevchenko changed the title ~~pkg/tracer: adding support for single span matching~~ WIP: pkg/tracer: adding support for single span matching Jun 27, 2022

pkg/tracer: added single span sampler

1f9b759

dianashevchenko changed the title ~~WIP: pkg/tracer: adding support for single span matching~~ pkg/tracer: adding support for single span matching Jun 29, 2022

pkg/tracer: added single span sampling at trace finish

d056ab1

dianashevchenko force-pushed the shevchenko/single-span-ingestion branch from 5abe908 to d056ab1 Compare June 29, 2022 15:19

pkg/tracer: filter out single spans to send to the agent if the decis…

bd20964

…ion isn't keep

ajgajg1134 requested changes Jun 29, 2022

View reviewed changes

katiehockman reviewed Jun 29, 2022

View reviewed changes

gbbr changed the title ~~pkg/tracer: adding support for single span matching~~ ddtrace/tracer: adding support for single span matching Jun 30, 2022

gbbr suggested changes Jun 30, 2022

View reviewed changes

gbbr reviewed Jun 30, 2022

View reviewed changes

dianashevchenko added 2 commits June 30, 2022 16:58

updated according to comments

408f38d

removed outdated test

f3de96c

dianashevchenko requested review from gbbr, katiehockman and ajgajg1134 July 1, 2022 12:33

Hellzy modified the milestones: 1.39.0, 1.40.0 Jul 1, 2022

dianashevchenko added 4 commits July 5, 2022 16:51

refactored according to comments Moved traceSampler and singleSpanSam…

a975fde

…pler to a new struct; moved traceSampler code to a separate file; updated sampler to include rules from WithSamplingRules option where both types of rules can be set;

fixed a test - wrong sampler method called

c465cae

reworded log message

db105b7

Merge branch 'main' into shevchenko/single-span-ingestion

95ee83b

dianashevchenko added 4 commits July 19, 2022 19:36

updated according to comments

1348caa

moved single span sampling to the worker part

4ce0635

Merge branch 'shevchenko/single-span-ingestion' of https://github.com…

9f73819

…/DataDog/dd-trace-go into shevchenko/single-span-ingestion

added docs to sampling rule struct

f33308b

dianashevchenko requested a review from gbbr July 20, 2022 17:44

dianashevchenko added 2 commits July 21, 2022 14:11

added partial tag to dropped_p0_traces metric

e4e6f3c

fixed flaky sampling test

fbe94ea

Merge branch 'main' into shevchenko/single-span-ingestion

2a625d5

ajgajg1134 requested changes Jul 26, 2022

View reviewed changes

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

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

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

updated doc comments; set sampling priority to 2; do not remove the t…

d295eee

…op level tag anymore

dianashevchenko requested a review from ajgajg1134 August 1, 2022 15:16

Merge branch 'main' into shevchenko/single-span-ingestion

1de6ded

knusbaum reviewed Aug 1, 2022

View reviewed changes

dianashevchenko added 3 commits August 1, 2022 18:51

changed Sampling Rule marshalling for better readability

58ff019

Merge branch 'shevchenko/single-span-ingestion' of https://github.com…

c0d7e02

…/DataDog/dd-trace-go into shevchenko/single-span-ingestion

fixed regex

46b0fe6

dianashevchenko force-pushed the shevchenko/single-span-ingestion branch from a9c49ce to 46b0fe6 Compare August 1, 2022 17:00

knusbaum previously approved these changes Aug 1, 2022

View reviewed changes

gbbr reviewed Aug 2, 2022

View reviewed changes

refactored according to comments

e8d62c7

dianashevchenko dismissed knusbaum’s stale review via e8d62c7 August 2, 2022 10:00

gbbr previously approved these changes Aug 2, 2022

View reviewed changes

removed unnecessary time.Sleep

bafbfd9

dianashevchenko dismissed gbbr’s stale review via bafbfd9 August 2, 2022 12:26

ajgajg1134 approved these changes Aug 2, 2022

View reviewed changes

Merge branch 'main' into shevchenko/single-span-ingestion

6dc3bc7

dianashevchenko merged commit d3776a9 into main Aug 2, 2022

dianashevchenko deleted the shevchenko/single-span-ingestion branch August 2, 2022 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddtrace/tracer: adding support for single span matching #1357

ddtrace/tracer: adding support for single span matching #1357

dianashevchenko commented Jun 27, 2022

ajgajg1134 left a comment

katiehockman left a comment

katiehockman Jun 29, 2022

gbbr left a comment

gbbr Jun 30, 2022

dianashevchenko Jun 30, 2022

gbbr Jun 30, 2022

gbbr Jun 30, 2022 •

edited

Loading

dianashevchenko Jun 30, 2022

gbbr Jun 30, 2022

dianashevchenko Jun 30, 2022

gbbr Jun 30, 2022

dianashevchenko commented Jul 21, 2022

ajgajg1134 left a comment

knusbaum Jul 7, 2022

gbbr left a comment

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr Aug 2, 2022

gbbr left a comment

gbbr left a comment

	func (rs singleSpanRulesSampler) applyRate(span span, rule SamplingRule, rate float64, now time.Time) {
	func (rs singleSpanRulesSampler) applyRule(span span, rule SamplingRule, now time.Time) {

		// traceInfo contains span list and sampling decision of the trace
		// needed to run single span sampling.

ddtrace/tracer: adding support for single span matching #1357

ddtrace/tracer: adding support for single span matching #1357

Conversation

dianashevchenko commented Jun 27, 2022

ajgajg1134 left a comment

Choose a reason for hiding this comment

katiehockman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianashevchenko commented Jul 21, 2022

ajgajg1134 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr left a comment

Choose a reason for hiding this comment

gbbr Jun 30, 2022 •

edited

Loading