Baggage support #4563

ida613 · 2024-07-31T13:51:26Z

What does this PR do?

Add support for baggage in datadog public api

Motivation

Plugin Checklist

Additional Notes

github-actions · 2024-07-31T13:52:14Z

Overall package size

Self size: 7.81 MB
Deduped: 64.67 MB
No deduping: 65.01 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 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.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 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 | | rfdc | 1.3.1 | 25.21 kB | 25.21 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 | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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-07-31T13:59:33Z

Benchmarks

Benchmark execution time: 2024-10-30 02:14:14

Comparing candidate commit f898c98 in PR branch ida613/baggage with baseline commit 1c0958e in branch master.

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

packages/dd-trace/src/config.js

packages/dd-trace/src/opentracing/span.js

codecov · 2024-08-05T23:38:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.77%. Comparing base (fd0f570) to head (1f3ebc0).
Report is 11 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4563       +/-   ##
===========================================
- Coverage   68.58%   54.77%   -13.81%     
===========================================
  Files          12      125      +113     
  Lines         818     3980     +3162     
===========================================
+ Hits          561     2180     +1619     
- Misses        257     1800     +1543

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

…ida613/baggage

rochdev

What happens when there is a conflict between a Datadog baggage and an OpenTelemetry baggage?

packages/dd-trace/src/config.js

rochdev · 2024-10-23T20:17:17Z

packages/dd-trace/src/opentracing/span_context.js

@@ -36,13 +36,15 @@ class DatadogSpanContext {
        ? this._trace.tags[TRACE_ID_128] + this._traceId.toString(16).padStart(16, '0')
        : this._traceId.toString(16).padStart(32, '0')
    }
+    // if (!this._traceId) return ''


Dead code should be removed and not just commented out.

ida613 · 2024-10-25T01:18:14Z

What happens when there is a conflict between a Datadog baggage and an OpenTelemetry baggage?

when the new baggage propagation style this PR proposes comes into conflict with existing baggage propagation, the existing style takes precedence. Is this what you are asking?

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

lucaspimentel · 2024-11-04T17:46:23Z

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

+      let buf = Buffer.from(baggage)
+      if (buf.length > this._config.baggageMaxBytes) {
+        const originalBaggages = baggage.split(',')
+        buf = buf.subarray(0, this._config.baggageMaxBytes)


While this code looks mostly correct, there's an edge case in buf.subarray(0, this._config.baggageMaxBytes) where we could theoretically split a multi-byte unicode character and end up with invalid UTF-8 bytes. I imagine if this happens the buf.toString('utf8') below would fail.

BUT, I don't think this will actually ever happen because we should never get multi-byte characters here, since we should have encoded all of them already with encodeURIComponent(key) above.

So now I'm thinking, if all characters in the string are guaranteed to be 1-byte ASCII characters after encoding, do we need even need Bugger.from(baggage) and buf.toString('utf8')? Or can we use the string's length directly without converting to UTF-8 bytes and back?

As a suggestion for my comment above, I think you can keep a count of bytes like the way you already keep a count of items, and follow similar logic. Something like

// consider this pseudo-code 😅 let baggage = '' let itemCounter = 0 let byteCounter = 0 for (const [key, value] of Object.entries(spanContext._baggageItems)) { let item = `${...},` // "key=value," itemCounter += 1 byteCounter += item.length // all characters are 1 byte after uri-encoding // stop adding baggage items if we reach either limit if (itemCounter > maxItems || byteCounter > maxBytes) break baggage += item } baggage = baggage.slice(0, baggage.length - 1) // ...

lucaspimentel · 2024-11-04T18:30:38Z

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

+    it('should handle special characters in baggage', () => {
+      const carrier = {}
+      const baggageItems = {
+        '",;\\()/:<=>?@[]{}': '",;\\'


Can we add non-ASCII characters like é or 我 or 🐶 to ensure those are encoded correctly, too?

(I thought I added this comment before, but I don't see it. Sorry if it's a duplicate.)

oops merged before I saw your comments, will create a separate PR to address your comments

lucaspimentel

I think we can simplify the code that handles the header's byte limit and add a few test cases, but aside from that LGTM!

otel baggage support

1d46ff2

ida613 added the semver-minor label Jul 31, 2024

ida613 added 3 commits July 31, 2024 14:26

modified otel baggage support

951e4cb

bug fix

d14fe44

removed undefined function

55c9880

lucaspimentel reviewed Jul 31, 2024

View reviewed changes

packages/dd-trace/src/config.js Outdated Show resolved Hide resolved

lucaspimentel reviewed Jul 31, 2024

View reviewed changes

packages/dd-trace/src/opentracing/span.js Outdated Show resolved Hide resolved

ida613 added 4 commits July 31, 2024 16:33

unit test

0d626b2

update implementation

87d8174

added encoding

f49cb62

add span formatting

700f022

ida613 added 16 commits August 6, 2024 14:56

revert format changes

c0f74d8

update config options

d6672cc

updated http encoding

d98648b

remove comments

7ffc64a

add noop and otel baggage support

4880dea

Merge branch 'master' into ida613/baggage

a4b90c7

updated according to rfc

b2ff71c

Merge branch 'master' into ida613/baggage

d1a2be3

fix unit test

49aaa7a

Merge branch 'ida613/baggage' of github.com:DataDog/dd-trace-js into …

d514bed

…ida613/baggage

Merge branch 'master' into ida613/baggage

32e91a7

update span api

8b9b917

logging debugging message

de79a4e

Merge branch 'master' into ida613/baggage

dae0866

bug fix

089353f

remove limits from baggage extraction

ac0f133

ida613 and others added 5 commits October 21, 2024 15:48

Merge branch 'master' into ida613/baggage

1f3ebc0

bug fix

9f1bfac

reverse yarn.lock changes

2e6afab

remove yarn.lock changes

cadc687

Merge branch 'master' into ida613/baggage

97ccd23

ida613 changed the title ~~WIP: otel baggage support proof of concept~~ WIP: Baggage support Oct 23, 2024

revert otel changes

1249935

ida613 changed the title ~~WIP: Baggage support~~ Baggage support Oct 23, 2024

ida613 marked this pull request as ready for review October 23, 2024 19:10

ida613 requested review from a team as code owners October 23, 2024 19:10

ida613 mentioned this pull request Oct 23, 2024

Modify baggage tests for nodejs + add associated nodejs endpoints DataDog/system-tests#3299

Draft

7 tasks

rochdev requested changes Oct 23, 2024

View reviewed changes

lucaspimentel reviewed Oct 28, 2024

View reviewed changes

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

ida613 and others added 3 commits October 29, 2024 21:47

removed dead code and updated baggage pair limits

c27f768

Merge branch 'master' into ida613/baggage

58d1b7f

fix linter errors

f898c98

rochdev approved these changes Nov 4, 2024

View reviewed changes

lucaspimentel reviewed Nov 4, 2024

View reviewed changes

lucaspimentel approved these changes Nov 4, 2024

View reviewed changes

ida613 merged commit f58e746 into master Nov 4, 2024
205 checks passed

ida613 deleted the ida613/baggage branch November 4, 2024 18:34

ida613 mentioned this pull request Nov 4, 2024

simplify baggage code and add test case #4858

Merged

6 tasks

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Baggage support (#4563)

078fd16

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Baggage support (#4563)

123703d

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Baggage support (#4563)

c065ae5

rochdev pushed a commit that referenced this pull request Nov 6, 2024

Baggage support (#4563)

0b89ccf

ida613 mentioned this pull request Nov 6, 2024

skip warning if propagator is baggage #4866

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Baggage support #4563

Baggage support #4563

ida613 commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

pr-commenter bot commented Jul 31, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading

rochdev left a comment

rochdev Oct 23, 2024

ida613 Oct 24, 2024

ida613 commented Oct 25, 2024 •

edited

Loading

lucaspimentel Nov 4, 2024 •

edited

Loading

lucaspimentel Nov 4, 2024 •

edited

Loading

lucaspimentel Nov 4, 2024

ida613 Nov 4, 2024

lucaspimentel left a comment

Baggage support #4563

Baggage support #4563

Conversation

ida613 commented Jul 31, 2024 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Jul 31, 2024 • edited Loading

Overall package size

pr-commenter bot commented Jul 31, 2024 • edited Loading

Benchmarks

codecov bot commented Aug 5, 2024 • edited Loading

Codecov Report

rochdev left a comment

Choose a reason for hiding this comment

rochdev Oct 23, 2024

Choose a reason for hiding this comment

ida613 Oct 24, 2024

Choose a reason for hiding this comment

ida613 commented Oct 25, 2024 • edited Loading

lucaspimentel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

lucaspimentel Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

lucaspimentel Nov 4, 2024

Choose a reason for hiding this comment

ida613 Nov 4, 2024

Choose a reason for hiding this comment

lucaspimentel left a comment

Choose a reason for hiding this comment

ida613 commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

pr-commenter bot commented Jul 31, 2024 •

edited

Loading

codecov bot commented Aug 5, 2024 •

edited

Loading

ida613 commented Oct 25, 2024 •

edited

Loading

lucaspimentel Nov 4, 2024 •

edited

Loading

lucaspimentel Nov 4, 2024 •

edited

Loading