feat(tracing): Add user data to `tracestate` header #3343

lobsterkatie · 2021-03-24T04:54:11Z

As the title says...

Specific things to note:

User data is included as an object, with the keys id and segment. For now, the id is sent un-hashed (until we can figure out exactly how we want to handle hashing in a way which is repeatable across platforms).
The sentry part of the tracestate header is now computed lazily, to increase the chances user data will make it in (since it often isn't know at transaction start time):
- If a sentry tracestate value is passed in at transaction creation time, as when it's inherited, that value is used and no amount of setting user data (or any other data, for that matter) after the fact will affect it. (This is not a change.)
- If no value is passed to the constructor, the property remains undefined until it's needed. (This is a change.)
- The first time it might be needed is to be used in headers on an outgoing request. Therefore, getTraceHeaders is now a get-or-create method with respect to the sentry tracestate value. Once it's been set in that method once, it is immutable. (This is also a change.)
- The second time it might be needed is for use in envelope headers for transaction events. If no sentry tracestate value is passed to the transaction constructor and no XHR requests are made during the transaction, the transaction will close without a sentry tracestate value. In this case, transaction.finish will compute the value as it's capturing the event. (This, too, is a change.)

github-actions · 2021-03-24T05:08:43Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	27.03 KB (-0.01% 🔽)
@sentry/browser - Webpack	27.91 KB (0%)
@sentry/react - Webpack	27.95 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	34.8 KB (+0.3% 🔺)

RaduW

From what I can figure out it looks reasonable.
There are a few things I didn't quite understand, see comments, but there is no stopper as far as I'm concerned.

RaduW · 2021-03-24T10:41:34Z

packages/tracing/src/span.ts

+import { getCurrentHub, Hub } from '@sentry/hub';
+import { Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';


What's going on here ? Doesn't seem to be related to the PR.

It actually is, though. The change here is from importing the type to importing the class. Later on in the file I'm calling Hub.getScope(), a method which exists on the class but isn't in the type, so if I don't make this change, it won't compile. The alternative to doing this is to add the method to the type, which I'm happy to do instead if you think that's better.

Understood, it's probably me (I'm not so familiar with this code) but it further raises the question why we don't have getScope in the interface ( since we do have pushScope, popScope, withScope).

Anyway, not the right place to discuss general architecture of the API.

packages/tracing/src/span.ts

packages/tracing/src/utils.ts

RaduW · 2021-03-24T15:51:16Z

packages/tracing/src/transaction.ts

+    // ensure that we have a tracestate to attach to the envelope header
+    if (!this.metadata.tracestate?.sentry) {
+      this.metadata.tracestate = { ...this.metadata.tracestate, sentry: this._getNewTracestate() };
+    }
+


Nit: I saw that this is not a new thing (it predates this PR) but why do we push tracestate in debug_meta ?
Nobody uses it in Relay from debug_meata (I'm looking for it in the http headers).

If we didn't _getNewTracestate() could become private.

why do we push tracestate in debug_meta ?
Nobody uses it in Relay from debug_meata (I'm looking for it in the http headers).

Because it needs to get through to here somehow, and putting it in debug_meta means that even if it slips through* and makes it to sentry, it won't show up for the user. (Previously the information hitched a ride on a custom tag, and a bug in the code meant that it was showing up for users.)

*If you look a few lines into the linked function, you'll see that in fact it gets pulled out of the eventual event payload. Same with the sampling data.

If we didn't _getNewTracestate() could become private.

Is there any advantage to that over it being protected (which it is currently)?

Again it was just a nit, but in general I find protected members to be a sign of a messy abstraction. It is something that you think you shouldn't expose ( otherwise you would make it public) but have to expose it "a little" because derived classes need to mess with it.

RaduW · 2021-03-24T15:55:59Z

packages/types/src/transaction.ts

+  /**
+   * Metadata about the transaction
+   */
+  metadata: TransactionMetadata;
+


why did we add this ?

Also looking more in detail at TransactionMetadata I understand that traceSampling can be interesting but I can't quite figure out why we pass tracestate since we can figure it out from the headers.

why did we add this ?

Because we now need to access it from within the Span class. Since the Transaction class extends the Span class, it imports from './span'. If the Span class also imported the Transaction class (from './transaction'), we'd end up in an import loop. Span therefore has to import Transaction instead from from @sentry/types, which means the type needs to know about the metadata property.

I can't quite figure out why we pass tracestate since we can figure it out from the headers

This is us having figured it out from the headers. The headers are passed to extractTracestateData, and the data then comes through startTransaction to the constructor (for example here.)

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 6 commits March 23, 2021 18:39

add metadata to Transaction type and segment to User type

ba466c9

move to lazy creation of tracestate value

86ac6a3

include user data in tracestate

a2ee549

when creating tracestate, get hub from transaction if possible

2572ff4

add tests

ba14834

change schema to match spec

2a7b830

lobsterkatie requested a review from kamilogorek as a code owner March 24, 2021 04:54

lobsterkatie requested review from RaduW and jan-auer March 24, 2021 04:54

RaduW approved these changes Mar 24, 2021

View reviewed changes

fix typo

7cc1d39

lobsterkatie merged commit 8e349e7 into kmclb-dynamic-sampling-data-in-envelopes Apr 2, 2021

lobsterkatie deleted the kmclb-add-user-to-tracestate branch April 2, 2021 22:39

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

feat(tracing): Add user data to tracestate header (#3343)

68331b9

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

feat(tracing): Add user data to tracestate header (#3343)

d2def39

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

Uh oh!

Uh oh!

feat(tracing): Add user data to `tracestate` header #3343

feat(tracing): Add user data to `tracestate` header #3343

Uh oh!

lobsterkatie commented Mar 24, 2021

Uh oh!

github-actions bot commented Mar 24, 2021 •

edited

Loading

Uh oh!

RaduW left a comment

Uh oh!

RaduW Mar 24, 2021

Uh oh!

lobsterkatie Mar 24, 2021 •

edited

Loading

Uh oh!

RaduW Mar 26, 2021

Uh oh!

Uh oh!

Uh oh!

RaduW Mar 24, 2021

Uh oh!

lobsterkatie Mar 24, 2021 •

edited

Loading

Uh oh!

RaduW Mar 26, 2021 •

edited

Loading

Uh oh!

RaduW Mar 24, 2021

Uh oh!

lobsterkatie Mar 24, 2021

Uh oh!

Uh oh!

		import { getCurrentHub, Hub } from '@sentry/hub';
		import { Primitive, Span as SpanInterface, SpanContext, TraceHeaders, Transaction } from '@sentry/types';

Uh oh!

feat(tracing): Add user data to tracestate header #3343

feat(tracing): Add user data to tracestate header #3343

Uh oh!

Conversation

lobsterkatie commented Mar 24, 2021

Uh oh!

github-actions bot commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

RaduW left a comment

Choose a reason for hiding this comment

Uh oh!

RaduW Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaduW Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RaduW Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaduW Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaduW Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(tracing): Add user data to `tracestate` header #3343

feat(tracing): Add user data to `tracestate` header #3343

github-actions bot commented Mar 24, 2021 •

edited

Loading

lobsterkatie Mar 24, 2021 •

edited

Loading

lobsterkatie Mar 24, 2021 •

edited

Loading

RaduW Mar 26, 2021 •

edited

Loading