implement w3c phase 3 #4325

khanayan123 · 2024-05-17T20:01:44Z

What does this PR do?

ensure tracecontext headers take precedence over all distributed tracing header formats

Motivation

Implement W3C phase 3

Additional Notes

Tested against these set of system tests:
https://github.com/DataDog/system-tests/blob/main/tests/parametric/test_headers_tracecontext.py#L717-%23L809

github-actions · 2024-05-17T20:02:25Z

Overall package size

Self size: 6.62 MB
Deduped: 61.87 MB
No deduping: 62.15 MB

Dependency sizes

name	version	self size	total size
@datadog/native-appsec	8.0.1	15.59 MB	15.6 MB
@datadog/native-iast-taint-tracking	2.1.0	14.91 MB	14.92 MB
@datadog/pprof	5.3.0	9.85 MB	10.22 MB
protobufjs	7.2.5	2.77 MB	6.56 MB
@datadog/native-iast-rewriter	2.3.1	2.15 MB	2.24 MB
@opentelemetry/core	1.14.0	872.87 kB	1.47 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
@opentelemetry/api	1.8.0	1.21 MB	1.21 MB
import-in-the-middle	1.7.4	70.19 kB	739.86 kB
msgpack-lite	0.1.26	201.16 kB	281.59 kB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.5.4	93.4 kB	123.8 kB
pprof-format	2.1.0	111.69 kB	111.69 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ignore	5.2.4	51.22 kB	51.22 kB
int64-buffer	0.1.10	49.18 kB	49.18 kB
shell-quote	1.8.1	44.96 kB	44.96 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
tlhunter-sorted-set	0.1.0	24.94 kB	24.94 kB
limiter	1.1.5	23.17 kB	23.17 kB
dc-polyfill	0.1.4	23.1 kB	23.1 kB
retry	0.13.1	18.85 kB	18.85 kB
jest-docblock	29.7.0	8.99 kB	12.76 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

pr-commenter · 2024-05-17T20:08:51Z

Benchmarks

Benchmark execution time: 2024-06-04 19:44:43

Comparing candidate commit eaeb59d in PR branch khanayan123/implement-w3c-phase3 with baseline commit 0e1f0cf in branch master.

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

codecov · 2024-05-17T21:37:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.09%. Comparing base (e60feae) to head (dea3ddf).
Report is 3 commits behind head on master.

❗ Current head dea3ddf differs from pull request most recent head eaeb59d

Please upload reports for the commit eaeb59d to get more accurate results.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4325       +/-   ##
===========================================
+ Coverage   69.19%   93.09%   +23.90%     
===========================================
  Files           1       97       +96     
  Lines         198     3028     +2830     
  Branches       33       33               
===========================================
+ Hits          137     2819     +2682     
- Misses         61      209      +148

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

khanayan123 · 2024-05-17T21:42:19Z

The systems tests cover more scenarios and they have been tested locally and are all passing. These are the system tests tested against: https://github.com/DataDog/system-tests/blob/main/tests/parametric/test_headers_tracecontext.py#L717-%23L809

mabdinur

left some nits. Overall looks really good

mabdinur · 2024-05-17T20:04:14Z

packages/dd-trace/src/opentracing/propagation/text_map.js

+    if (spanContext !== null) {
+      return spanContext
    }

    return this._extractSqsdContext(carrier)


Can we simplify this to:

Suggested change

if (spanContext !== null) {

return spanContext

}

return this._extractSqsdContext(carrier)

return spanContext || this._extractSqsdContext(carrier)

mabdinur · 2024-05-20T14:33:20Z

packages/dd-trace/src/opentracing/propagation/text_map.js

+        if (w3cCtx !== null && spanContext.toTraceId(true) === w3cCtx.toTraceId(true) &&
+        spanContext.toSpanId() !== w3cCtx.toSpanId()) {
+          if ('_dd.parent_id' in w3cCtx._trace.tags && w3cCtx._trace.tags['_dd.parent_id'] !==
+          '0000000000000000') {


Should we make 0000000000000000 and '_dd.parent_id' constants?

mabdinur · 2024-05-20T14:34:00Z

packages/dd-trace/src/opentracing/propagation/text_map.js

+
+        const w3cCtx = this._extractTraceparentContext(carrier)
+
+        if (w3cCtx !== null && spanContext.toTraceId(true) === w3cCtx.toTraceId(true) &&


Should we add a comment to document this behavior or encapsulate it in a method.

mabdinur

Looks good from the API team perspective. Make sure you run the W3C system tests locally before merging the PR

mabdinur · 2024-05-20T17:25:03Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -214,9 +216,43 @@ class TextMapPropagator {
    return this._config.tracePropagationStyle[mode].includes(name)
  }

+  _ensureTraceContextTakePrecedence (traceContext, spanContext, carrier) {


nit: We can make these parameters a little more clear

Suggested change

_ensureTraceContextTakePrecedence (traceContext, spanContext, carrier) {

_ensureTraceContextTakePrecedence (w3cSpanContext, firstSpanContext, carrier) {

mabdinur · 2024-05-20T17:27:45Z

packages/dd-trace/src/opentracing/propagation/text_map.js

+        if (extractor !== 'tracecontext') {
+          continue
+        }
+        spanContext = this._ensureTraceContextTakePrecedence(


nit: Tracecontext only takes precedence if we detect a conflicting spans in the same trace. We should try to make that clear (with a common or in the function name).

Suggested change

spanContext = this._ensureTraceContextTakePrecedence(

spanContext = this._resolveTraceContextConflicts(

rochdev · 2024-05-21T16:28:22Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -214,9 +216,43 @@ class TextMapPropagator {
    return this._config.tracePropagationStyle[mode].includes(name)
  }

+  _resolveTraceContextConflicts (w3cSpanContext, firstSpanContext, carrier) {


This function would be a lot easier to read if split up with smaller functions with representative names that would also replace comments. For example hasTraceParent etc.

rochdev · 2024-05-21T16:34:26Z

packages/dd-trace/test/opentracing/propagation/text_map.spec.js

@@ -523,6 +523,19 @@ describe('TextMapPropagator', () => {
      expect(spanContext._tracestate).to.be.undefined
    })

+    it('extracts span_id from tracecontext headers and stores datadog parent-id in trace_distributed_tags', () => {


There was a lot of new code with many branches added. Should there be more tests for all combinations?

All the branches should be tested by the following system tests: https://github.com/DataDog/system-tests/blob/main/tests/parametric/test_headers_tracecontext.py#L717-%23L809 (including the scenario in this unit test).

We are trying to avoid writing the same test cases in 8 different languages.

I understand the goal, but with that approach, how can we know that things continue to work while working locally? And how do we monitor code coverage?

Yup we can add one more test and test should give us full code coverage. We can use the system tests to address edges with unexpected inputs.

Added a new unit test to address all branch's

tlhunter · 2024-06-04T17:45:50Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -39,6 +39,8 @@ const tracestateTagKeyFilter = /[^\x21-\x2b\x2d-\x3c\x3e-\x7e]/g
 // Tag values in tracestate replace ',', '~' and ';' with '_'
 const tracestateTagValueFilter = /[^\x20-\x2b\x2d-\x3a\x3c-\x7d]/g
 const invalidSegment = /^0+$/
+const ddParentIdTagKey = '_dd.parent_id'
+const zeroHex = '0000000000000000'


The variable name is a bit too literal IMO. For example, if I had a button with a warning color of "FF0000", which is red, the literal name would be const colorRed but a more descriptive name would be const colorWarning.

What is zeroHex really? Is it the trace ID of a parentless span? If so, a more descriptive name like "rootSpanTraceId" would be better. Otherwise using zeroHex is no better than '0000000000000000' in the code.

I like zeroTraceId for this.

tlhunter · 2024-06-04T17:47:23Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -39,6 +39,8 @@ const tracestateTagKeyFilter = /[^\x21-\x2b\x2d-\x3c\x3e-\x7e]/g
 // Tag values in tracestate replace ',', '~' and ';' with '_'
 const tracestateTagValueFilter = /[^\x20-\x2b\x2d-\x3a\x3c-\x7d]/g
 const invalidSegment = /^0+$/
+const ddParentIdTagKey = '_dd.parent_id'


Nit: There's a tags.js file that lists common tag names. Should entries like these be listed in that file? /cc @bengl

Yes it should. @khanayan123 can you please make that change?

bengl · 2024-06-04T18:58:17Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -39,6 +39,8 @@ const tracestateTagKeyFilter = /[^\x21-\x2b\x2d-\x3c\x3e-\x7e]/g
 // Tag values in tracestate replace ',', '~' and ';' with '_'
 const tracestateTagValueFilter = /[^\x20-\x2b\x2d-\x3a\x3c-\x7d]/g
 const invalidSegment = /^0+$/
+const ddParentIdTagKey = '_dd.parent_id'


Yes it should. @khanayan123 can you please make that change?

bengl · 2024-06-04T18:59:19Z

packages/dd-trace/src/opentracing/propagation/text_map.js

@@ -39,6 +39,8 @@ const tracestateTagKeyFilter = /[^\x21-\x2b\x2d-\x3c\x3e-\x7e]/g
 // Tag values in tracestate replace ',', '~' and ';' with '_'
 const tracestateTagValueFilter = /[^\x20-\x2b\x2d-\x3a\x3c-\x7d]/g
 const invalidSegment = /^0+$/
+const ddParentIdTagKey = '_dd.parent_id'
+const zeroHex = '0000000000000000'


I like zeroTraceId for this.

* implent w3c phase 3

khanayan123 requested a review from mabdinur May 17, 2024 20:02

khanayan123 changed the title ~~implent w3c phase 3~~ implement w3c phase 3 May 17, 2024

khanayan123 added the semver-minor label May 17, 2024

khanayan123 force-pushed the khanayan123/implement-w3c-phase3 branch from 3386657 to c6143f3 Compare May 17, 2024 20:05

mabdinur reviewed May 20, 2024

View reviewed changes

khanayan123 requested a review from mabdinur May 20, 2024 17:04

mabdinur reviewed May 20, 2024

View reviewed changes

khanayan123 requested a review from mabdinur May 20, 2024 17:44

rochdev reviewed May 21, 2024

View reviewed changes

mabdinur previously approved these changes May 24, 2024

View reviewed changes

khanayan123 dismissed mabdinur’s stale review via dea3ddf May 28, 2024 16:58

khanayan123 requested review from rochdev and mabdinur May 28, 2024 17:34

khanayan123 force-pushed the khanayan123/implement-w3c-phase3 branch 2 times, most recently from 17ffb42 to d7f9b96 Compare June 3, 2024 22:42

khanayan123 marked this pull request as ready for review June 4, 2024 14:59

khanayan123 requested a review from a team as a code owner June 4, 2024 14:59

tlhunter reviewed Jun 4, 2024

View reviewed changes

bengl previously approved these changes Jun 4, 2024

View reviewed changes

khanayan123 dismissed bengl’s stale review via 7723d97 June 4, 2024 19:33

khanayan123 added 4 commits June 4, 2024 15:34

implent w3c phase 3

7c8da06

compares 128 bits for trace id

8cd559e

address feedback

9132258

address nits

3101462

khanayan123 added 4 commits June 4, 2024 15:34

increase unit test coverage

6336e84

split up code into smaller more representative functions

7a196bb

remove comment

ceb0151

address feedback

eaeb59d

khanayan123 force-pushed the khanayan123/implement-w3c-phase3 branch from 7723d97 to eaeb59d Compare June 4, 2024 19:34

khanayan123 requested review from tlhunter and bengl June 4, 2024 19:43

bengl approved these changes Jun 4, 2024

View reviewed changes

khanayan123 merged commit 7b2dee1 into master Jun 4, 2024
133 checks passed

khanayan123 deleted the khanayan123/implement-w3c-phase3 branch June 4, 2024 19:52

juan-fernandez pushed a commit that referenced this pull request Jun 5, 2024

implement w3c phase 3 (#4325)

05eeb70

* implent w3c phase 3

juan-fernandez pushed a commit that referenced this pull request Jun 5, 2024

implement w3c phase 3 (#4325)

0a0726a

* implent w3c phase 3

juan-fernandez pushed a commit that referenced this pull request Jun 5, 2024

implement w3c phase 3 (#4325)

4ac7345

* implent w3c phase 3

juan-fernandez pushed a commit that referenced this pull request Jun 5, 2024

implement w3c phase 3 (#4325)

6754187

* implent w3c phase 3

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement w3c phase 3 #4325

implement w3c phase 3 #4325

khanayan123 commented May 17, 2024 •

edited

Loading

github-actions bot commented May 17, 2024 •

edited

Loading

pr-commenter bot commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

khanayan123 commented May 17, 2024 •

edited

Loading

mabdinur left a comment

mabdinur May 17, 2024

mabdinur May 20, 2024

mabdinur May 20, 2024

mabdinur left a comment

mabdinur May 20, 2024

mabdinur May 20, 2024

rochdev May 21, 2024

rochdev May 21, 2024

mabdinur May 21, 2024

rochdev May 21, 2024

mabdinur May 24, 2024

khanayan123 Jun 4, 2024

tlhunter Jun 4, 2024

bengl Jun 4, 2024

tlhunter Jun 4, 2024

bengl Jun 4, 2024

bengl Jun 4, 2024

bengl Jun 4, 2024


		const w3cCtx = this._extractTraceparentContext(carrier)

		if (w3cCtx !== null && spanContext.toTraceId(true) === w3cCtx.toTraceId(true) &&

	_ensureTraceContextTakePrecedence (traceContext, spanContext, carrier) {
	_ensureTraceContextTakePrecedence (w3cSpanContext, firstSpanContext, carrier) {

	spanContext = this._ensureTraceContextTakePrecedence(
	spanContext = this._resolveTraceContextConflicts(

implement w3c phase 3 #4325

implement w3c phase 3 #4325

Conversation

khanayan123 commented May 17, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

github-actions bot commented May 17, 2024 • edited Loading

Overall package size

Dependency sizes

pr-commenter bot commented May 17, 2024 • edited Loading

Benchmarks

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

khanayan123 commented May 17, 2024 • edited Loading

mabdinur 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

mabdinur 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

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

khanayan123 commented May 17, 2024 •

edited

Loading

github-actions bot commented May 17, 2024 •

edited

Loading

pr-commenter bot commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

khanayan123 commented May 17, 2024 •

edited

Loading