ddtrace/tracer/textmap: Setting _dd.propagation_error tag on faulty context propagation by dianashevchenko · Pull Request #1969 · DataDog/dd-trace-go

dianashevchenko · 2023-05-09T09:55:05Z

What does this PR do?

This PR recognises faulty context propagation when dd_p.tid (TraceID) is inconsistent or malformed

wrong length
non-hex characters
isn't consistent with the traceID passed from traceParent (W3C case scenario)

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-05-09T10:07:58Z

Benchmarks

Comparing candidate commit 738be73 in PR branch shevchenko/dd_propagation-error with baseline commit c9996a7 in branch main.

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

ajgajg1134

overall looks good! Just a few comments in the tests mainly

ajgajg1134 · 2023-05-10T13:46:59Z

 	}
-	if ctx.trace != nil {
+	if ctx.trace != nil && ctx.trace.propagatingTag(keyTraceID128) != "" {
 		// TODO: this always assumed it was valid so I copied that logic here, maybe we shouldn't


looks like we can remove this comment now?

ajgajg1134 · 2023-05-10T13:48:10Z


+func TestMalformedTIDPropagationErrorTag(t *testing.T) {
+	assert := assert.New(t)
+	t.Run("datadog", func(t *testing.T) {


Is there a more descriptive name for this and the next test?

ajgajg1134 · 2023-05-10T13:50:18Z

+		root := tracer.StartSpan("web.request", ChildOf(sctx)).(*span)
+		root.Finish()
+		assert.Contains(root.Meta, keyPropagationError)
+		assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXXXXXXXXXXXXXX")


Suggested change

assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXXXXXXXXXXXXXX")

assert.Equal("malformed_tid XXXXXXXXXXXXXXXX", root.Meta[keyPropagationError])

ajgajg1134 · 2023-05-10T13:51:01Z

+		root := tracer.StartSpan("web.request", ChildOf(sctx)).(*span)
+		root.Finish()
+		assert.Contains(root.Meta, keyPropagationError)
+		assert.Equal(root.Meta[keyPropagationError], "inconsistent_tid 640cfd8d0000ffff")


Suggested change

assert.Equal(root.Meta[keyPropagationError], "inconsistent_tid 640cfd8d0000ffff")

assert.Equal("inconsistent_tid 640cfd8d0000ffff", root.Meta[keyPropagationError])

ajgajg1134 · 2023-05-10T13:51:16Z

+		root := tracer.StartSpan("web.request", ChildOf(sctx)).(*span)
+		root.Finish()
+		assert.Contains(root.Meta, keyPropagationError)
+		assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXX")


Suggested change

assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXX")

assert.Equal("malformed_tid XXXX", root.Meta[keyPropagationError])

ajgajg1134 · 2023-05-10T13:56:49Z

+	assert := assert.New(t)
+	t.Run("datadog", func(t *testing.T) {
+		t.Setenv(headerPropagationStyleExtract, "datadog")
+		tracer := newTracer(WithPropagator(NewPropagator(nil)))


defer tracer.Stop() in these tests to ensure we cleanup the goroutines started that are associated with them

katiehockman · 2023-05-10T14:21:56Z

+			if len(ctx.trace.propagatingTag(keyTraceID128)) != 16 {
+				ctx.trace.unsetPropagatingTag(keyTraceID128)
+			}


Can we do this below where we're doing the hex parsing instead? Checking the validity of keyTraceID128 in two places makes this a bit more complicated than it needs to be. Plus, we also want to unset this propagating tag if the hex is invalid, so the logic should be the same. Right now, we're still keeping the _dd.p.tid tag even if it's invalid hex, which isn't correct per the RFC:

if the tag value is a valid 32 character lower-case hexadecimal encoded number, set the higher-order 64 bits of the TraceId to its (decoded) value.
if the tag value is malformed, discard the tag value and, optionally, set the dd.propagation_error tag to:
malformed_tid + <_dd.p.tid value>

katiehockman · 2023-05-10T14:24:56Z

+				if key == "t.tid" {
+					if ok := validIDRgx.MatchString(val); !ok {
+						ctx.trace.setTag(keyPropagationError, fmt.Sprintf("malformed_tid %s", val))
+					} else if val != ctx.traceID.UpperHex() {
+						ctx.trace.setTag(keyPropagationError, fmt.Sprintf("inconsistent_tid %s", val))
+					}
+				}


(From Slack convo) this is all optional, right? Based on the RFC and comments, it sounds like it's perfectly valid to just not propagate/use the _dd.p.tid tag when using W3C trace context propagation. If this is optional, then let's not do this unless we have a very compelling reason that it'll help customers. The code is already fairly complicated, so let's avoid doing more than we need to.

i.e. you can just add a continue right here instead if key == "t.tid" to not propagate it.

I'm not sure I see what you mean. Is that what you suggest ? :

if key == "t.tid" && (val != ctx.traceID.UpperHex() || (!validIDRgx.MatchString(val))) { continue } setPropagatingTag(key, val)

ahmed-mez · 2023-06-29T14:13:42Z

We already shipped the fix in #2076 and decided to not add the _dd.propagation_error tag so I'm closing this PR.

dianashevchenko added 2 commits May 5, 2023 15:46

added _dd.propagation_error tests

0d0b091

added a test case for valid hex-wrong length scenario

5506a61

dianashevchenko requested a review from a team May 9, 2023 09:55

dianashevchenko and others added 2 commits May 9, 2023 11:59

lint

56d84cc

Merge branch 'main' into shevchenko/dd_propagation-error

fa1a06f

dianashevchenko mentioned this pull request May 9, 2023

Enable tests for Go OTel and 128 bits work DataDog/system-tests#1160

Merged

ajgajg1134 changed the title ~~Setting _dd.propagation_error tag on faulty context propagation~~ ddtrace/tracer/textmap: Setting _dd.propagation_error tag on faulty context propagation May 10, 2023

ajgajg1134 requested changes May 10, 2023

View reviewed changes

katiehockman reviewed May 10, 2023

View reviewed changes

removed setting the propagation error

738be73

ahmed-mez mentioned this pull request Jun 26, 2023

tracer: discard invalid _dd.p.tid in the datadog propagator #2076

Merged

2 tasks

ahmed-mez closed this Jun 29, 2023

	assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXXXXXXXXXXXXXX")
	assert.Equal("malformed_tid XXXXXXXXXXXXXXXX", root.Meta[keyPropagationError])

	assert.Equal(root.Meta[keyPropagationError], "inconsistent_tid 640cfd8d0000ffff")
	assert.Equal("inconsistent_tid 640cfd8d0000ffff", root.Meta[keyPropagationError])

	assert.Equal(root.Meta[keyPropagationError], "malformed_tid XXXX")
	assert.Equal("malformed_tid XXXX", root.Meta[keyPropagationError])

Uh oh!

Conversation

dianashevchenko commented May 9, 2023

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

Uh oh!

pr-commenter Bot commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

ajgajg1134 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianashevchenko May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahmed-mez commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pr-commenter Bot commented May 9, 2023 •

edited

Loading

dianashevchenko May 12, 2023 •

edited

Loading