fix(tracing): Remove undefined tracestate data rather than setting it to null
#3373
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
Relay should perform all the validations anyway, so it should be fine imo. |
kamilogorek
reviewed
Apr 7, 2021
| @@ -227,7 +225,7 @@ describe('tracestate', () => { | |||
| }); | |||
|
|
|||
| it("won't include data set after first call to `getTraceHeaders`", () => { | |||
| expect.assertions(2); | |||
| expect.assertions(1); | |||
There was a problem hiding this comment.
There are no need to explicitly state number of assertions in sync tests, but it's fine to leave it. Personal preference.
There was a problem hiding this comment.
^^^ Not from this PR (I hadn't rebased yet) but I did the expect.assertions() because the expects are in a callback. Less of an issue in this test than some of the others, because the callback is explicitly called and included in the test itself, unlike, for instance, this one:
it('uses existing sentry tracestate in envelope headers rather than creating a new one', () => {
// one here, and two inside the `captureEvent` implementation above
expect.assertions(3);
kamilogorek
approved these changes
Apr 7, 2021
lobsterkatie
force-pushed
the
kmclb-stop-bundling-buffer-module
branch
2 times, most recently
from
00cfee2 to
9228f1e
Compare
Base automatically changed from
kmclb-stop-bundling-buffer-module
to
kmclb-dynamic-sampling-data-in-envelopes
April 12, 2021 18:56
lobsterkatie
force-pushed
the
kmclb-stop-sending-nulls-when-data-unavailable
branch
from
2a1806e to
44a6857
Compare
lobsterkatie
added a commit
that referenced
this pull request
Apr 12, 2021
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only two values guaranteed to be included in a transaction's
tracestatearetrace_idandpublic_key. The rest -release,environment,user.id, anduser.segment- may or may not have values. Previously, any of these lacking a value were being set tonullin thetracestateobject. This changes it so the keys are removed instead.The only snag I could foresee here is that if no user data is available, I've chosen to not send a
userkey at all, rather than sendinguser: { }. That means that on the relay end, asking foruser.idwill have to first include a check foruser. If this is a problem, I can switch to sending an empty object.NB: Currently this is a PR against another PR's branch, just so that only this PR's changes show up. Once that PR is merged, I'll rebase onto the main feature branch.