-
Notifications
You must be signed in to change notification settings - Fork 306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: otel baggage support proof of concept #4563
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 7.59 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.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 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 |
packages/dd-trace/src/config.js
Outdated
this._setValue(defaults, 'baggagePropagation', true) | ||
this._setValue(defaults, 'baggageInject', true) | ||
this._setValue(defaults, 'baggageExtract', true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was that the separate inject
/extract
settings take precedence over the third setting if they are explicitly set by the user. I don't know how you would handle this in Node.js, but you may want to set the default for those two as undefined
so yo can distinguish between a true
set by the user and a true
that was the default value.
The idea is that in most cases users only need to set DD_TRACE_BAGGAGE_ENABLED
and DD_TRACE_BAGGAGE_[INJECT|EXTRACT]_ENABLED
are only used in edge cases.
This is similar to what we do with
// used for both extract and inject below if those are not set explicitly
DD_TRACE_PROPAGATION_STYLE
overrides DD_TRACE_PROPAGATION_STYLE for extraction only
DD_TRACE_PROPAGATION_STYLE_EXTRACT
overrides DD_TRACE_PROPAGATION_STYLE for injection only
DD_TRACE_PROPAGATION_STYLE_INJECT
@@ -145,6 +145,18 @@ class DatadogSpan { | |||
return this._spanContext._baggageItems[key] | |||
} | |||
|
|||
getAllBaggageItems () { | |||
return this._spanContext._baggageItems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 functions LGTM. My only question is, should we return _baggageItems
directly here, or instead return an immutable copy, or something like an iterator? The OpenTelemetry spec says we should, but I don't think we necessarily need to follow that in the this API. Are there any concerns with exposing the internal _baggageItems
to users? This may vary between languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I'll return an immutable copy to be safe
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
What does this PR do?
Motivation
Plugin Checklist
Additional Notes