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

lobsterkatie · 2021-08-20T06:23:36Z

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)

(This isn't a literal rebasing - the commits in this PR are different from those above - because in each original commit there were so many merge conflicts, in so many parts of so many different files, that it seemed easier to apply the final state of that branch to current master and back-engineer a commit history from there, so that I only had to detangle each section of code once.)

Of all the TODOs included in those PR descriptions, the ones that remain relevant are:

We don't currently handle weird edge cases in terms of third-party data in the headers (blank values, for instance), as detailed in the official spec.
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).

In the meantime, one new one has cropped up:

We need to add transaction name to the tracestate data. In doing this, we'll need to note all of the places where a transaction's name changes partway through (or at the end of) its lifecycle, as this could mean that the value in the propagated tracestate (depending on when it's computed) might not match the final value on the transaction event itself. (Most often, these midstream name changes are the result of replacing a literal URL (/dogs/maisey) with a parameterized one (/dogs/:name), which can't be done until the framework has finished routing the request.) It's an open question what to do with such a mismatch, but at minimum we should know when it might happen.

The only other notable thing about the branch is that there's a fairly comprehensive description of tracestate lazy computation (which might be useful for the develop docs) included in the description for #3343.

github-actions · 2021-08-20T06:36:17Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	21.87 KB (+1.38% 🔺)
@sentry/browser - Webpack	22.83 KB (+1.1% 🔺)
@sentry/react - Webpack	22.86 KB (+1.11% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	30.02 KB (+3.43% 🔺)

rhcarvalho · 2021-08-20T15:12:24Z

packages/utils/test/string.test.ts

+  const base64String = 'RG9ncyBhcmUgZ3JlYXQh';
+
+  test('converts to valid base64', () => {
+    expect(BASE64_REGEX.test(unicodeToBase64(unicodeString))).toBe(true);


Isn't this assertion redundant when we already test for equality in the next lines?

The equality test passing implies this one will, it's true, but not vice-versa. My thinking here is that if the equality test fails but this test passes, it narrows down the problem.

rhcarvalho · 2021-08-20T15:14:58Z

packages/utils/test/string.test.ts

+  });
+
+  test('conversion functions are inverses', () => {
+    expect(base64ToUnicode(unicodeToBase64(unicodeString))).toEqual(unicodeString);


This is logically implied from the assertion above it.

I suppose that's true. I can remove it. (I'm not going to push the change until after the ember test problem is fixed, though, because I'm just going to have to rebase on top of that fix and push again at that point.)

rhcarvalho

Spotty review from mobile on the go. Sorry sorry

Afraid this is such a large commit touching 50+ files ... Maybe could have some of those trivial changes touching many test files be a separate PR...

rhcarvalho · 2021-08-20T15:19:36Z

packages/tracing/src/utils.ts

+  try {
+    return unicodeToBase64(JSON.stringify(dropUndefinedKeys(data))).replace(/={1,2}$/, '');
+  } catch (err) {
+    throw new SentryError(`[Tracing] Error computing tracestate value from data: ${err}\nData: ${data}`);


Hmm this could lead to a spammy stream of errors sent to Sentry if clients are sending bad data. It would be a safer approach here to ignore invalid input, might log the error to the console.

Users can't really fix this error once they see it in Sentry. Either our SDKs have a bug or a bad client is sending data and SDK users can't really fix it on their own.

I don't think it'll get through to users' issue streams. Because it's a SentryError, it'll just get thrown rather than captured - see

sentry-javascript/packages/core/src/baseclient.ts

Lines 542 to 559 in e46ff3e

this._sendEvent(processedEvent);

return processedEvent;

})

.then(null, reason => {

if (reason instanceof SentryError) {

throw reason;

}

this.captureException(reason, {

data: {

__sentry__: true,

},

originalException: reason as Error,

});

throw new SentryError(

`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,

);

});

.

rhcarvalho · 2021-08-20T15:23:19Z

packages/core/src/baseclient.ts

-    if (!('environment' in event)) {
-      event.environment = 'environment' in options ? environment : 'production';
+    if (event.environment === undefined && environment !== undefined) {
+      event.environment = environment;


Is this related to tracestate?

Yes. Environment is part of the data that goes into tracestate, so it needs to be set in time for that to happen. If the user provides an environment, we're already good, but if we need to use the default, at the moment that's not getting added until we're sending the event. This moves it earlier so that it's available when we compute the tracestate value.

See #3242.

rhcarvalho · 2021-08-20T15:25:34Z

packages/core/test/lib/base.test.ts

@@ -571,7 +547,6 @@ describe('BaseClient', () => {
      expect(TestBackend.instance!.event!).toEqual({
        breadcrumbs: [normalizedBreadcrumb, normalizedBreadcrumb, normalizedBreadcrumb],
        contexts: normalizedObject,
-        environment: 'production',


These test changes seen unrelated to tracestate

See my comment above re: setting default environment earlier.

lobsterkatie · 2021-08-20T15:37:28Z

Afraid this is such a large commit touching 50+ files ... Maybe could have some of those trivial changes touching many test files be a separate PR...

It was separate PRs - all of the ones listed in the PR description - all of which have already been reviewed. The only reason this is a new PR and not just a straight-up rebase of the branch is because it was getting too messy. So if there are glaring problems I'm happy to fix them, but anything other than that should probably be handled in a separate PR, since this is meant to be equivalent to having done the rebase directly.

…ent processing

lobsterkatie · 2021-08-23T23:28:27Z

Given that this has all been reviewed as part of the old PRs, and that it's getting merged into a feature branch rather than master, I'm going to merge this. If there's anything else which needs fixing, it can be done in the final PR merging the feature branch into master.

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 requested a review from kamilogorek as a code owner August 20, 2021 06:23

lobsterkatie force-pushed the kmclb-rebase-tracestate-proof-of-concept branch from a3be155 to 3bf477f Compare August 20, 2021 06:24

rhcarvalho reviewed Aug 20, 2021

View reviewed changes

lobsterkatie added 21 commits August 23, 2021 14:06

add TextEncoder and TextDecoder to jsdom test environment

9cfe4c7

add TransactionMetadata to DebugMeta type

37b4944

add segment to user type

2ec4722

add TraceHeaders and SpanContext.getTraceHeaders() to types

f9ba96c

add base64 string functions

bbf472d

fix circular dependency

9474a4e

move setting default environment to SDK initialization rather than ev…

6728b26

…ent processing

remove unused traceHeaders function

0af6717

polyfill Object.fromEntries for tracing tests

5e57193

s/TRACEPARENT_REGEXP/SENTRY_TRACE_REGEX

5d53567

s/extractTraceparentData/extractSentrytraceData

2a50338

s/traceparent/sentrytrace in various spots

54ac0d6

add functions for computing tracestate and combined tracing headers

58eeda9

add tracestate header to outgoing requests

a758e16

deprecate toTraceparent

a007a83

add extractTracestateData() function

1a4336e

make transactions accept metadata in their incoming transaction context

fcb41b3

propagate incoming tracestate headers

f0f7ed6

extract and propagate tracestate data from <meta> tags

9ab5a8f

add missing tracestate when capturing transaction, if necessary

08d9ba4

only deal with envelope header data if we're using an envelope

5c6070e

lobsterkatie added 7 commits August 23, 2021 14:06

add tracestate to envelope header

50819b6

clean up comments

756e855

add http header tests

a0b5caf

random cleanup

f1a0dbf

don't stringify event data until the end

6415a5a

rework and add request tests

15a2e85

remove redundant string test, add browser test for non-base64 string

dd76840

lobsterkatie force-pushed the kmclb-rebase-tracestate-proof-of-concept branch from 3bf477f to dd76840 Compare August 23, 2021 21:06

lobsterkatie force-pushed the kmclb-rebase-tracestate-proof-of-concept branch from 6b1fbfa to dd76840 Compare August 23, 2021 23:35

lobsterkatie merged commit 8f9cb49 into kmclb-add-tracestate-header-handling Aug 24, 2021

lobsterkatie deleted the kmclb-rebase-tracestate-proof-of-concept branch August 24, 2021 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

lobsterkatie commented Aug 20, 2021

Uh oh!

github-actions bot commented Aug 20, 2021 •

edited

Loading

Uh oh!

rhcarvalho Aug 20, 2021

Uh oh!

lobsterkatie Aug 20, 2021

Uh oh!

rhcarvalho Aug 20, 2021

Uh oh!

lobsterkatie Aug 20, 2021

Uh oh!

rhcarvalho left a comment

Uh oh!

rhcarvalho Aug 20, 2021

Uh oh!

lobsterkatie Aug 20, 2021 •

edited

Loading

Uh oh!

rhcarvalho Aug 20, 2021

Uh oh!

lobsterkatie Aug 20, 2021

Uh oh!

rhcarvalho Aug 20, 2021

Uh oh!

lobsterkatie Aug 20, 2021

Uh oh!

lobsterkatie commented Aug 20, 2021 •

edited

Loading

Uh oh!

lobsterkatie commented Aug 23, 2021

Uh oh!

Uh oh!

	this._sendEvent(processedEvent);
	return processedEvent;
	})
	.then(null, reason => {
	if (reason instanceof SentryError) {
	throw reason;
	}

	this.captureException(reason, {
	data: {
	__sentry__: true,
	},
	originalException: reason as Error,
	});
	throw new SentryError(
	`Event processing pipeline threw an error, original event will not be sent. Details have been sent as a new event.\nReason: ${reason}`,
	);
	});

Uh oh!

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

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

Uh oh!

Conversation

lobsterkatie commented Aug 20, 2021

Uh oh!

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

Uh oh!

size-limit report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhcarvalho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Aug 20, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

lobsterkatie commented Aug 23, 2021

Uh oh!

Uh oh!

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

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

github-actions bot commented Aug 20, 2021 •

edited

Loading

lobsterkatie Aug 20, 2021 •

edited

Loading

lobsterkatie commented Aug 20, 2021 •

edited

Loading