Skip to content

fix(tracing): Use Request.headers.append correctly #3311

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

Conversation

lobsterkatie
Copy link
Member

Oh, the difference a pair or two of brackets can make.

For no reason in the world, I read the existing (currently on master) call as
headers.append([["header-name", "header-value"]])
when in fact it's
headers.append("header-name", "header-value").
The first version makes feeding .append() the results of Object.entries() a reasonable thing to do. The latter... does not.

This fixes the call, and adds tests specifically hitting each of the four branches of the surrounding conditional (which my original tests had not).

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 26.88 KB (+0.01% 🔺)
@sentry/browser - Webpack 27.81 KB (0%)
@sentry/react - Webpack 27.84 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 34.55 KB (+0.09% 🔺)

@lobsterkatie lobsterkatie merged commit 6364402 into kmclb-dynamic-sampling-data-in-envelopes Mar 5, 2021
@lobsterkatie lobsterkatie deleted the kmclb-fix-trace-header-appending branch March 5, 2021 14:16
lobsterkatie added a commit that referenced this pull request Aug 24, 2021
This is the result of rebasing the feature branch for the initial implementation of `tracestate` header handling (which had grown very stale) on top of current `master`. That branch is going to get deleted, so for posterity, it included the following PRs (oldest -> newest):

feat(tracing): Add dynamic sampling correlation context data to envelope header (#3062)
chore(utils): Split browser/node compatibility utils into separate module (#3123)
ref(tracing): Prework for initial `tracestate` implementation (#3242)
feat(tracing): Add `tracestate` header to outgoing requests (#3092)
ref(tracing): Rework tracestate internals to allow for third-party data (#3266)
feat(tracing): Handle incoming tracestate data, allow for third-party data (#3275)
chore(tracing): Various small fixes to first `tracestate` implementation (#3291)
fix(tracing): Use `Request.headers.append` correctly (#3311)
feat(tracing): Add user data to tracestate header (#3343)
chore(various): Small fixes (#3368)
fix(build): Prevent Node's `Buffer` module from being included in browser bundles (#3372)
fix(tracing): Remove undefined tracestate data rather than setting it to `null` (#3373)

More detail in the PR description.
lobsterkatie added a commit that referenced this pull request Aug 30, 2021
This is the result of rebasing the feature branch for the initial implementation of `tracestate` header handling (which had grown very stale) on top of current `master`. That branch is going to get deleted, so for posterity, it included the following PRs (oldest -> newest):

feat(tracing): Add dynamic sampling correlation context data to envelope header (#3062)
chore(utils): Split browser/node compatibility utils into separate module (#3123)
ref(tracing): Prework for initial `tracestate` implementation (#3242)
feat(tracing): Add `tracestate` header to outgoing requests (#3092)
ref(tracing): Rework tracestate internals to allow for third-party data (#3266)
feat(tracing): Handle incoming tracestate data, allow for third-party data (#3275)
chore(tracing): Various small fixes to first `tracestate` implementation (#3291)
fix(tracing): Use `Request.headers.append` correctly (#3311)
feat(tracing): Add user data to tracestate header (#3343)
chore(various): Small fixes (#3368)
fix(build): Prevent Node's `Buffer` module from being included in browser bundles (#3372)
fix(tracing): Remove undefined tracestate data rather than setting it to `null` (#3373)

More detail in the PR description.
lobsterkatie added a commit that referenced this pull request Aug 31, 2021
This is the result of rebasing the feature branch for the initial implementation of `tracestate` header handling (which had grown very stale) on top of current `master`. That branch is going to get deleted, so for posterity, it included the following PRs (oldest -> newest):

feat(tracing): Add dynamic sampling correlation context data to envelope header (#3062)
chore(utils): Split browser/node compatibility utils into separate module (#3123)
ref(tracing): Prework for initial `tracestate` implementation (#3242)
feat(tracing): Add `tracestate` header to outgoing requests (#3092)
ref(tracing): Rework tracestate internals to allow for third-party data (#3266)
feat(tracing): Handle incoming tracestate data, allow for third-party data (#3275)
chore(tracing): Various small fixes to first `tracestate` implementation (#3291)
fix(tracing): Use `Request.headers.append` correctly (#3311)
feat(tracing): Add user data to tracestate header (#3343)
chore(various): Small fixes (#3368)
fix(build): Prevent Node's `Buffer` module from being included in browser bundles (#3372)
fix(tracing): Remove undefined tracestate data rather than setting it to `null` (#3373)

More detail in the PR description.
lobsterkatie added a commit that referenced this pull request Sep 15, 2021
This is the result of rebasing the feature branch for the initial implementation of `tracestate` header handling (which had grown very stale) on top of current `master`. That branch is going to get deleted, so for posterity, it included the following PRs (oldest -> newest):

feat(tracing): Add dynamic sampling correlation context data to envelope header (#3062)
chore(utils): Split browser/node compatibility utils into separate module (#3123)
ref(tracing): Prework for initial `tracestate` implementation (#3242)
feat(tracing): Add `tracestate` header to outgoing requests (#3092)
ref(tracing): Rework tracestate internals to allow for third-party data (#3266)
feat(tracing): Handle incoming tracestate data, allow for third-party data (#3275)
chore(tracing): Various small fixes to first `tracestate` implementation (#3291)
fix(tracing): Use `Request.headers.append` correctly (#3311)
feat(tracing): Add user data to tracestate header (#3343)
chore(various): Small fixes (#3368)
fix(build): Prevent Node's `Buffer` module from being included in browser bundles (#3372)
fix(tracing): Remove undefined tracestate data rather than setting it to `null` (#3373)

More detail in the PR description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants