ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

Barbayar · 2021-12-06T15:17:19Z

Changes

Introduced a notion of trace level tags.
Introduced a notion of propagated trace level tags.
Introduced a new trace level tag _dd.p.upstream_services that holds the services changed sampling decisions.

ddtrace/tracer/span.go

ddtrace/tracer/payload.go

ddtrace/tracer/textmap.go

ddtrace/tracer/payload.go

ddtrace/tracer/spancontext.go

ddtrace/tracer/span.go

internal/appsec/waf.go

ddtrace/tracer/sampler.go

gbbr · 2021-12-07T12:07:47Z

ddtrace/tracer/spancontext.go

@@ -217,7 +235,13 @@ func (t *trace) setSamplingPriorityLocked(p float64) {
 	if t.priority == nil {
 		t.priority = new(float64)
 	}
-	*t.priority = p
+	*t.priority = float64(p)
+	encodedService := b64Encode(service)


I think we should only keep of the current priority, rate and the sampler name, and avoid re-encoding this every time.

At the very end, we just append to this tag.

ddtrace/ext/tags.go

ddtrace/tracer/payload.go

ddtrace/tracer/spancontext.go

ddtrace/tracer/textmap.go

ddtrace/tracer/util.go

internal/appsec/waf.go

ddtrace/tracer/textmap.go

internal/appsec/waf.go

won't break the current appsec spec

ddtrace/tracer/span.go

gbbr

I think we're getting closer to a final implementation.

ddtrace/tracer/option.go

ddtrace/tracer/sampler.go

ddtrace/tracer/span.go

gbbr · 2021-12-16T14:06:20Z

ddtrace/tracer/span.go

@@ -283,10 +304,14 @@ func (s *span) setMetric(key string, v float64) {
 	}
 	delete(s.Meta, key)
 	switch key {
+	case ext.ManualKeep:
+		if v == float64(samplerAppSec) {


This is quite nasty. But I can't think of a better approach.

ddtrace/tracer/span.go

ddtrace/tracer/spancontext.go

ddtrace/tracer/option.go

gbbr

Looks good to me, just a few nits. Would love it if @knusbaum could also take a look, just in case he notices something I didn't.

ddtrace/tracer/option.go

ddtrace/tracer/sampler.go

gbbr · 2022-01-04T14:06:05Z

ddtrace/tracer/sampler.go

+	// and it's noop except propagating the decision.
+	samplerNone samplerName = math.MinInt8
+	// samplerUnknown specifies that the span was sampled
+	// but, the tracer was unable to identify the sampler.


How is this possible? In what scenarios?

This is not used in Go tracer. This is reserved value for other tracers in case they are not able to find out who triggered the sampling decision. I can remove it to avoid confusions. Wdyt?

It looks like it's never used in the code, but it is in the RFC. I'm OK keeping it here for completeness. The RFC document doesn't say anything about when it can happen...

gbbr · 2022-01-04T14:06:30Z

ddtrace/tracer/sampler.go

+	// samplerUnknown specifies that the span was sampled
+	// but, the tracer was unable to identify the sampler.
+	samplerUnknown samplerName = -1
+	// samplerDefault specifies that the span was sampled without any sampler.


How is this different from samplerUnknown? These 3 are still confusing and the documentation doesn't clarify the confusion.

This was supposed to be used until the sampling rates come from the agent for the first time.
But, I didn't implement that logic in this PR. We can do it later.
I can remove samplerDefault from this PR now, then add back when we implement that logic.

ddtrace/tracer/sampler.go

ddtrace/tracer/spancontext.go

knusbaum

Looks mostly good. I have a few comments.

knusbaum · 2022-01-05T16:52:54Z

ddtrace/tracer/span.go

+	case ext.ManualKeep:
+		if v == float64(samplerAppSec) {
+			s.setSamplingPriorityLocked(ext.PriorityUserKeep, samplerAppSec, math.NaN())
+		}


Looks like this is here so we can set ManualKeep with a particular sampler (span.SetTag(ext.ManualKeep, samplerAppSec))

Doesn't this case cause ext.ManualKeep to not be set unless by the samplerAppSec? Do we want ext.ManualKeep to work for other samplers? That would be a lot more consistent rather than having this special case.

Doesn't this case cause ext.ManualKeep to not be set unless by the samplerAppSec?

No, span.SetTag(ext.ManualKeep, true) still works as before.

Do we want ext.ManualKeep to work for other samplers?

I think we shouldn't support/allow customer specifying the sampler freely. This was a small hack for AppSec, because AppSec library cannot access private properties/methods of the tracer.

No, span.SetTag(ext.ManualKeep, true) still works as before.

I know that still works. What I mean is that setting manual keep with any other sampler value will silently fail, and unless I read the setMetric code, I have no way to know that. This will be surprising behavior if I see appsec setting the sampler with this mechanism.

If we want to do this hack, we should expose samplerAppSec as some internal constant so that appsec can use it. I guess it would have to be in github.com/DataDog/dd-trace-go.v1/internal. We should also document the mechanism (not in the public docs) so we know what works (only samplerAppSec)

ddtrace/tracer/spancontext.go

ddtrace/tracer/option.go

ddtrace/tracer/span_test.go

ddtrace/tracer/textmap.go

internal/appsec/waf.go

ddtrace/tracer/sampler.go

knusbaum · 2022-01-12T15:34:36Z

ddtrace/tracer/sampler.go

+	// and it's noop except propagating the decision.
+	samplerNone samplerName = math.MinInt8
+	// samplerUnknown specifies that the span was sampled
+	// but, the tracer was unable to identify the sampler.


It looks like it's never used in the code, but it is in the RFC. I'm OK keeping it here for completeness. The RFC document doesn't say anything about when it can happen...

ddtrace/tracer/textmap.go

knusbaum · 2022-01-12T15:52:35Z

ddtrace/tracer/span.go

+	case ext.ManualKeep:
+		if v == float64(samplerAppSec) {
+			s.setSamplingPriorityLocked(ext.PriorityUserKeep, samplerAppSec, math.NaN())
+		}


No, span.SetTag(ext.ManualKeep, true) still works as before.

I know that still works. What I mean is that setting manual keep with any other sampler value will silently fail, and unless I read the setMetric code, I have no way to know that. This will be surprising behavior if I see appsec setting the sampler with this mechanism.

If we want to do this hack, we should expose samplerAppSec as some internal constant so that appsec can use it. I guess it would have to be in github.com/DataDog/dd-trace-go.v1/internal. We should also document the mechanism (not in the public docs) so we know what works (only samplerAppSec)

knusbaum

Just one more nitpick from me.

knusbaum · 2022-01-14T15:00:04Z

internal/appsec/waf.go

+// samplerAppSec specifies that the span was sampled by AppSec.
+samplerAppSec int8 = 5
+


Let's point at ddtrace/tracer/sampler.go here if we're not going to share the definitions. Otherwise I have no idea of the context of 5.

internal/appsec/dyngo/instrumentation/grpcsec/tags.go

Julio-Guerra · 2022-01-17T09:48:09Z

ddtrace/tracer/sampler.go

+	// samplerManual specifies that the span was sampled manually by user.
+	samplerManual samplerName = 4
+	// samplerAppSec specifies that the span was sampled by AppSec.
+	samplerAppSec samplerName = 5


@knusbaum, @Barbayar: given this value is used by AppSec but is currently hard-coded there, WDYT of exporting it /internal/something so that /contrib/**/appsec.go can use the same const value?
I fear this value changes at some point without changing them in appsec, and conversely.

👍 I suggested the same thing. If others agree, let's go ahead and move all of these.

Julio-Guerra

Changing the state of the PR to a non-approved state.

Julio-Guerra

All good regarding internal/appsec/* 👍

knusbaum

Just one comment, but not a blocker (it's not something you changed).

Everything looks good. 👍

knusbaum · 2022-01-24T21:13:25Z

ddtrace/tracer/util.go

+	case int8:
+		return float64(i), true


It's surprising that this was missing. We should probably go ahead and also add uint8 for completeness.

This commit reverts part of: ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

Barbayar changed the title ~~[Draft] Propagates _dd.p.upstream_services~~ [WIP] Propagates _dd.p.upstream_services Dec 6, 2021

Barbayar commented Dec 6, 2021

View reviewed changes

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

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

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

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

Barbayar added this to the 1.35.0 milestone Dec 6, 2021

Barbayar commented Dec 7, 2021

View reviewed changes

ddtrace/tracer/spancontext.go Show resolved Hide resolved

Barbayar commented Dec 7, 2021

View reviewed changes

ddtrace/tracer/span.go Show resolved Hide resolved

Barbayar commented Dec 7, 2021

View reviewed changes

internal/appsec/waf.go Outdated Show resolved Hide resolved

gbbr reviewed Dec 8, 2021

View reviewed changes

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from 8294fc2 to 1fb68a2 Compare December 8, 2021 13:49

gbbr reviewed Dec 9, 2021

View reviewed changes

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

Barbayar commented Dec 9, 2021

View reviewed changes

internal/appsec/waf.go Outdated Show resolved Hide resolved

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from 035ca27 to 197acb8 Compare December 13, 2021 12:49

Barbayar commented Dec 13, 2021

View reviewed changes

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

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from afa8ecc to 6dff09c Compare December 13, 2021 16:09

Barbayar changed the title ~~[WIP] Propagates _dd.p.upstream_services~~ Propagates _dd.p.upstream_services Dec 13, 2021

Barbayar marked this pull request as ready for review December 13, 2021 19:36

Barbayar requested a review from gbbr December 13, 2021 19:37

Julio-Guerra previously requested changes Dec 13, 2021

View reviewed changes

internal/appsec/waf.go Outdated Show resolved Hide resolved

felixge modified the milestones: 1.35.0, 1.36.0 Dec 14, 2021

Julio-Guerra self-requested a review December 14, 2021 16:15

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from bfc09a1 to 3573402 Compare December 15, 2021 23:14

gbbr changed the title ~~Propagates _dd.p.upstream_services~~ ddtrace/tracer: propagate _dd.p.upstream_services tags Dec 16, 2021

gbbr reviewed Dec 16, 2021

View reviewed changes

ddtrace/tracer/span.go Show resolved Hide resolved

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from b333743 to 93efac7 Compare December 16, 2021 14:40

gbbr reviewed Dec 16, 2021

View reviewed changes

Barbayar commented Dec 18, 2021

View reviewed changes

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

Barbayar requested a review from knusbaum December 21, 2021 10:27

gbbr reviewed Jan 4, 2022

View reviewed changes

knusbaum requested changes Jan 5, 2022

View reviewed changes

Barbayar requested a review from a team as a code owner January 12, 2022 12:09

Barbayar requested review from a team, knusbaum and gbbr January 12, 2022 12:09

knusbaum requested changes Jan 12, 2022

View reviewed changes

knusbaum requested changes Jan 14, 2022

View reviewed changes

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch 2 times, most recently from 9293b0b to 44f2399 Compare January 14, 2022 20:08

Barbayar requested a review from knusbaum January 14, 2022 20:19

knusbaum previously approved these changes Jan 14, 2022

View reviewed changes

internal/appsec/dyngo/instrumentation/grpcsec/tags.go Outdated Show resolved Hide resolved

Julio-Guerra reviewed Jan 17, 2022

View reviewed changes

Julio-Guerra requested changes Jan 24, 2022

View reviewed changes

Barbayar added 5 commits January 24, 2022 12:22

Implemented _dd.p.upstream_services tag

be568c3

updated AppSec's ext.ManualKeep usage

435949f

fixed unit tests

87db826

fixed AppSec sampler

008cc44

moved samplerNames to an internal package

0a4ded8

Barbayar dismissed knusbaum’s stale review via 0a4ded8 January 24, 2022 12:41

Barbayar force-pushed the barbayar.dashzeveg/trace-tags-propagation branch from 44f2399 to 0a4ded8 Compare January 24, 2022 12:41

Barbayar requested review from Julio-Guerra and knusbaum January 24, 2022 12:50

Julio-Guerra approved these changes Jan 24, 2022

View reviewed changes

knusbaum approved these changes Jan 24, 2022

View reviewed changes

knusbaum merged commit e52ef96 into v1 Jan 24, 2022

knusbaum deleted the barbayar.dashzeveg/trace-tags-propagation branch January 24, 2022 22:38

martin-sucha mentioned this pull request Feb 23, 2022

WARN: won't propagate trace tags (err: max trace tags header len (0) reached) #1180

Open

knusbaum pushed a commit that referenced this pull request Feb 25, 2022

ddtrace/tracer: disable Datadog internal tag propagation (#1182)

4f0b6ac

This commit reverts part of: ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

knusbaum pushed a commit that referenced this pull request Mar 1, 2022

ddtrace/tracer: disable Datadog internal tag propagation (#1182)

fb10734

This commit reverts part of: ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

This was referenced Sep 26, 2024

v2: remove deprecated samplingpriority #2890

Closed

v2: remove sampling priority #2906

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

Barbayar commented Dec 6, 2021 •

edited

Loading

gbbr Dec 7, 2021

gbbr left a comment

gbbr Dec 16, 2021

gbbr left a comment

gbbr Jan 4, 2022

Barbayar Jan 12, 2022 •

edited

Loading

knusbaum Jan 12, 2022

gbbr Jan 4, 2022

Barbayar Jan 12, 2022

knusbaum left a comment

knusbaum Jan 5, 2022

Barbayar Jan 12, 2022

knusbaum Jan 12, 2022

knusbaum Jan 12, 2022

knusbaum Jan 12, 2022

knusbaum left a comment

knusbaum Jan 14, 2022

Julio-Guerra Jan 17, 2022

knusbaum Jan 18, 2022

Julio-Guerra left a comment

Julio-Guerra left a comment

knusbaum left a comment

knusbaum Jan 24, 2022

		// samplerAppSec specifies that the span was sampled by AppSec.
		samplerAppSec int8 = 5

ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

ddtrace/tracer: propagate _dd.p.upstream_services tags #1082

Conversation

Barbayar commented Dec 6, 2021 • edited Loading

Changes

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

gbbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barbayar Jan 12, 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

knusbaum 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

knusbaum 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

Julio-Guerra left a comment

Choose a reason for hiding this comment

Julio-Guerra left a comment

Choose a reason for hiding this comment

knusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barbayar commented Dec 6, 2021 •

edited

Loading

Barbayar Jan 12, 2022 •

edited

Loading