ref(tracing): Rework tracestate internals to allow for third-party data #3266

lobsterkatie · 2021-02-17T05:32:23Z

There's no behavior change here - just a bunch of shifting things around so that

tracestate data lives inside the new (since the previous PR's inception) metadata property on the Transaction class rather than directly on the transaction,
the data is stored as an object rather than a string, so there will be somewhere to put other vendors' data,
the value stored includes the sentry= (rather than just being the value) because that will match the may the third-party data is stored, and
orphan spans can generate tracestate headers, too.

…evel property

lobsterkatie · 2021-02-17T05:34:57Z

packages/tracing/src/transaction.ts

+    this.metadata = transactionContext.metadata || {};
+    this.metadata.tracestate = this.metadata.tracestate || {};
+    this.metadata.tracestate.sentry = this.metadata.tracestate.sentry || this._getNewTracestate(this._hub);


Is there a nicer/more compact way to do this? This feels like a lot of code, but I don't know of a setter version of the ?. getter syntax.

No, not really, but we do this in so many places, that I might actually write

getOrDefault(this, 'metadata.tracestate.sentry', this._getNewTracestate(this._hub))

helper for this :<

Like that https://lodash.com/docs/4.17.15#get

Yes, except we also need to set the data. I think a helper isn't a bad idea, but I'll save that for a separate PR.

github-actions · 2021-02-17T05:47:26Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	26.88 KB (+0.06% 🔺)
@sentry/browser - Webpack	27.81 KB (+0.06% 🔺)
@sentry/react - Webpack	27.83 KB (+0.06% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	34.18 KB (+0.13% 🔺)

kamilogorek

Other than this one comment, 👍

kamilogorek · 2021-02-17T08:34:31Z

packages/core/src/request.ts

  try {
-    tracestate = JSON.parse(base64ToUnicode(encodedTracestate as string));
+    const encodedSentryValue = (tracestate?.sentry as string).replace('sentry=', '');
+    reinflatedTracestate = JSON.parse(base64ToUnicode(encodedSentryValue));
  } catch (err) {
    logger.warn(err);


This would log Cannot read property 'replace' of undefined for situations when there's simply no tracestate.sentry which might be confusing to the user.

True. Originally I had it as const encodedSentryValue = tracestate!.sentry!.replace('sentry=', '');, since if we get to that point and there's no tracestate.sentry, things have gone real wrong, but then it was yelling at me about the !. But I think maybe I should change it back, to better communicate to the reader that it's not optional.

Or maybe I can write my types in a smarter way... In any case, will fix it one way or another.

UPDATE: Okay, tried with the types, but there are too many stages at which this or that property may (legitimately) not be defined, so I'm going with the !.

…ta (#3266)

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 7 commits February 16, 2021 21:24

add TransactionMetadata type to @sentry/types

d955dba

move method for getting new header from Transaction class to Span class

f70e334

store sentry tracestate value in metadata object rather than as top-l…

b4e8b8a

…evel property

let orphan spans get tracestate headers also

628d33a

fix envelope headers

2124f20

clean up tracestate computation function

1df7bfb

fix linting issues

1ee03f3

lobsterkatie requested a review from kamilogorek as a code owner February 17, 2021 05:32

lobsterkatie commented Feb 17, 2021

View reviewed changes

fix test

29a8e02

kamilogorek approved these changes Feb 17, 2021

View reviewed changes

clarify that tracestate.sentry should exist

dd45062

lobsterkatie merged commit 78251cc into kmclb-dynamic-sampling-data-in-envelopes Feb 17, 2021

lobsterkatie deleted the kmclb-pre-third-party-tracestate-refactor branch February 17, 2021 17:47

lobsterkatie added a commit that referenced this pull request Feb 23, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

5b0d405

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Feb 23, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

bc27310

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Feb 23, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

965013c

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Mar 1, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

edf0735

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Mar 15, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

c2bf385

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Apr 7, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

7f06486

…ta (#3266)

lobsterkatie added a commit that referenced this pull request Apr 12, 2021

ref(tracing): Rework tracestate internals to allow for third-party da…

b5a0cb4

…ta (#3266)

lobsterkatie mentioned this pull request Aug 20, 2021

feat(tracing): Add initial tracestate header handling #3909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(tracing): Rework tracestate internals to allow for third-party data #3266

ref(tracing): Rework tracestate internals to allow for third-party data #3266

lobsterkatie commented Feb 17, 2021

lobsterkatie Feb 17, 2021

kamilogorek Feb 17, 2021 •

edited

Loading

kamilogorek Feb 17, 2021

lobsterkatie Feb 17, 2021

github-actions bot commented Feb 17, 2021 •

edited

Loading

kamilogorek left a comment

kamilogorek Feb 17, 2021

lobsterkatie Feb 17, 2021 •

edited

Loading

ref(tracing): Rework tracestate internals to allow for third-party data #3266

ref(tracing): Rework tracestate internals to allow for third-party data #3266

Conversation

lobsterkatie commented Feb 17, 2021

lobsterkatie Feb 17, 2021

Choose a reason for hiding this comment

kamilogorek Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

kamilogorek Feb 17, 2021

Choose a reason for hiding this comment

lobsterkatie Feb 17, 2021

Choose a reason for hiding this comment

github-actions bot commented Feb 17, 2021 • edited Loading

size-limit report

kamilogorek left a comment

Choose a reason for hiding this comment

kamilogorek Feb 17, 2021

Choose a reason for hiding this comment

lobsterkatie Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

kamilogorek Feb 17, 2021 •

edited

Loading

github-actions bot commented Feb 17, 2021 •

edited

Loading

lobsterkatie Feb 17, 2021 •

edited

Loading