Skip to content
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

skip warning if propagator is baggage #4866

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

ida613
Copy link
Collaborator

@ida613 ida613 commented Nov 6, 2024

What does this PR do?

skip warning for the unconventional propagator baggage. Lucas gave a good explanation for having baggage as a propagator.

Motivation

Plugin Checklist

Additional Notes

@ida613 ida613 requested a review from a team as a code owner November 6, 2024 18:39
Copy link

github-actions bot commented Nov 6, 2024

Overall package size

Self size: 7.94 MB
Deduped: 64.96 MB
No deduping: 65.3 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 | 3.0.1 | 1.06 MB | 1.46 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
Copy link

pr-commenter bot commented Nov 6, 2024

Benchmarks

Benchmark execution time: 2024-11-07 15:10:45

Comparing candidate commit 58c40c3 in PR branch ida613/baggage-propagator-skip-warning with baseline commit 1ee8000 in branch master.

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

@@ -324,7 +324,7 @@ class TextMapPropagator {
spanContext = this._extractB3MultiContext(carrier)
break
default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're already in a switch statement based on the extractor value you could make this a lot cleaner by doing this:

        case 'baggage':
          break
        default:
          log.warn(`Unknown propagation style: ${extractor}`)

Copy link
Collaborator Author

@ida613 ida613 Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of awkward having baggage as a propagator, if we go with this cleaner solution (which also makes more sense to me), in the case we have this._config.tracePropagationStyle.extract = ['baggage', 'datadog'], we would end up with spanContext without traceId or parentId which the datadog propagation style clearly requires. Having baggage as a propagator simply means we are propagating baggage, but it should not override other propagation styles.

@ida613 ida613 merged commit 36903cc into master Nov 13, 2024
205 of 206 checks passed
@ida613 ida613 deleted the ida613/baggage-propagator-skip-warning branch November 13, 2024 20:00
rochdev pushed a commit that referenced this pull request Nov 13, 2024
@rochdev rochdev mentioned this pull request Nov 13, 2024
rochdev pushed a commit that referenced this pull request Nov 13, 2024
@rochdev rochdev mentioned this pull request Nov 13, 2024
rochdev pushed a commit that referenced this pull request Nov 19, 2024
rochdev pushed a commit that referenced this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants