-
Notifications
You must be signed in to change notification settings - Fork 329
add new baggage APIs which act on currently active baggage #5365
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
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 9.33 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.1 | 29.73 MB | 29.73 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 3.3.1 | 13.99 MB | 13.99 MB | | @datadog/pprof | 5.7.1 | 9.51 MB | 9.88 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/wasm-js-rewriter | 4.0.0 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 kB | | source-map | 0.7.4 | 226 kB | 226 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.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.8 | 25.08 kB | 25.08 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5365 +/- ##
==========================================
- Coverage 79.47% 79.37% -0.10%
==========================================
Files 509 515 +6
Lines 22811 23437 +626
==========================================
+ Hits 18128 18603 +475
- Misses 4683 4834 +151 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 930 Passed, 0 Skipped, 14m 54.99s Total Time |
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 transport itself seems fine, I just don't see how this is integrated and exposed to users. That is something that still needs to be addressed :)
Should I update our api documentation? Are there other ways we can expose global functions to users? |
@BridgeAR Hey Ruben, I reached out to Rachel, who implemented context-based public baggage APIs for golang, and there is really nothing left to do except maybe for adding the APIs to the public doc. Are you able to give this PR a go? Thank you :) |
@BridgeAR sorry about misunderstanding you before! I have added the APIs to NoopProxy to make them accessible to the public. Please let me know if this solution is fine and I'll update the test :) |
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.
Thanks for exposing the API now!
While it would work that way, it should not be on the no op
proxy class. The name is an indicator for no operation, so all methods on that class should just do nothing. Instead, the API should probably be exposed on the main Tracer proxy.
Can you please also add tests that actually use the API as an integration test and also add documentation for it?
@BridgeAR thank you for your review Ruben! :) This is awkward, technically baggage is separate from tracing and does not require tracing to be enabled to work |
The methods should be added to the proxy in |
Unfortunately the original design of the library was only a tracer (hence the name dd-trace) and the default export ended up being the tracer itself. This means that anything, whether or not related to tracing, needs to be on the tracer to be exposed. Of course that could be directly on the proxy instead of the underlying tracer, but from a public API perspective they are the effectively the same. The underlying implementation however should indeed not be coupled to tracing. |
Made changes as Bryan suggested! Will work on tests now |
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 code is looking good to me!
I believe we still need some documentation for it. Otherwise it would be pretty much "in the wild" :D
I believe our TS definition is pretty much that documentation, if I am not mistaken.
@BridgeAR added the TS definition! let me know if it's good to go :) |
What does this PR do?
Provides users with a set of global functions which act on the currently active baggage of implicitly propagated context
Motivation
From the baggage RFC:
Added return statements because
Plugin Checklist
Additional Notes