feat(tracing): Handle incoming tracestate data, allow for third-party data #3275

lobsterkatie · 2021-02-23T06:56:35Z

This builds on previous dynamic sampling/tracestate PRs by (as the title says), consuming incoming tracestate headers and allowing for non-sentry data in both those and outgoing tracestate headers.

This should complete the initial implementation of tracestate on the JavaScript side.

Lingering issues to be dealt with in future:

Because Buffer is mentioned in utils/string.ts, webpack will include the entire contents of node's Buffer module in your bundle when you include @sentry/browser or any of its derivatives in said bundle. (Rollup is smart enough to exclude it from our CDN bundles.) Before we GA this, we need to figure out a way around that. Holding off for now since all of the build tooling is changing in v7, and this likely won't GA until after v7 is released. (As to our dogfooding, @HazAT suggested seeing how much of an impact it actually has there before spending time dealing with it.)
This initial proof of concept doesn't include any user data, which the final version will need to.
This also doesn't handle weird edge cases in terms of third-party data in the headers (blank values, for instance), as detailed in the official spec.

github-actions · 2021-02-23T09:57:43Z

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.83 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	34.59 KB (+1.02% 🔺)

lobsterkatie · 2021-02-24T18:47:12Z

The linting error is solved in a follow-up cleanup PR to come once this is merged into the feature branch.

HazAT

LGTM, skimmed over it at it makes sense

packages/serverless/src/awslambda.ts

kamilogorek · 2021-02-26T13:52:05Z

packages/tracing/src/browser/browsertracing.ts

+  const sentrytraceData = sentrytraceValue ? extractSentrytraceData(sentrytraceValue) : undefined;
+  const tracestateData = tracestateValue ? extractTracestateData(tracestateValue) : undefined;
+
+  if (sentrytraceData || tracestateData?.sentry || tracestateData?.thirdparty) {


tracestateData?.thirdparty is useless here, as it will never be reached due to the first sentrytraceData check already shorting it with its truthiness.

But sentrytraceData isn't guaranteed to be truthy - if the sentry-trace <meta> tag isn't there, or if the value is somehow malformed, you could have tracestate data without having sentry-trace data. Similarly, if we have a tracesatate value in which there's no sentry portion (or the sentry portion is malformed), then we could have third-party tracestate data without having sentry tracestate data. So I think we need all three checks, since the existence of each value is independent of the existence of the others.

packages/tracing/src/utils.ts

kamilogorek · 2021-02-26T13:57:37Z

packages/tracing/src/utils.ts

+
+      // remove the commas after the split so we don't end up with `xxx=yyy,,zzz=qqq` (double commas) when we put them
+      // back together
+      [before, after] = header.split(sentryEntry).map(s => s.replace(/^,*|,*$/g, ''));


Suggested change

[before, after] = header.split(sentryEntry).map(s => s.replace(/^,*|,*$/g, ''));

[before, after] = header.split(sentryEntry).map(s => s.replace(/,/g, ''));

This should be good enough I guess. We don't allow for commas anywhere in the middle of a value anyway.

Actually, no. That's what I had originally, but if the header comes in with a value like

x=y,sentry=blah,a=b,c=d

then getting rid of all commas gets rid of the comma between a=b and c=d, which isn't what we want.

packages/tracing/src/utils.ts

… data (#3275)

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 force-pushed the kmclb-handle-third-party-tracestate branch from 01f2b1c to b808fcc Compare February 23, 2021 20:58

lobsterkatie added 7 commits February 24, 2021 10:08

standardize naming

da6fccf

add method to extract tracestate data from header

ae47cea

add thirdparty data to tracestate type

52f6d98

more renaming

ab72f62

create toTracestate method

934be68

inherit thidparty tracestate when given

91fda65

make toTracestate private

269a678

lobsterkatie force-pushed the kmclb-handle-third-party-tracestate branch from 4a57c05 to fbd3c0f Compare February 24, 2021 18:16

lobsterkatie added 5 commits February 24, 2021 10:21

add deprecation warning to toTraceparent and alias to private method

f6fadf8

add tracestate <meta> tag handling

81c6193

read tracestate data from incoming node requests

33290ad

add todo to span tests

f46c8c4

make serverless handle incoming tracestate headers

06e8d3b

lobsterkatie force-pushed the kmclb-handle-third-party-tracestate branch from fbd3c0f to 06e8d3b Compare February 24, 2021 18:29

lobsterkatie marked this pull request as ready for review February 24, 2021 18:47

lobsterkatie requested a review from kamilogorek as a code owner February 24, 2021 18:47

HazAT approved these changes Feb 26, 2021

View reviewed changes

kamilogorek reviewed Feb 26, 2021

View reviewed changes

packages/serverless/src/awslambda.ts Show resolved Hide resolved

kamilogorek reviewed Feb 26, 2021

View reviewed changes

packages/tracing/src/utils.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Feb 26, 2021

View reviewed changes

packages/tracing/src/utils.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Feb 26, 2021

View reviewed changes

packages/tracing/src/utils.ts Outdated Show resolved Hide resolved

apply code review fixes

e5cea2e

lobsterkatie merged commit 4cf7262 into kmclb-dynamic-sampling-data-in-envelopes Feb 27, 2021

lobsterkatie deleted the kmclb-handle-third-party-tracestate branch February 27, 2021 02:22

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

feat(tracing): Handle incoming tracestate data, allow for third-party…

4d519db

… data (#3275)

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

feat(tracing): Handle incoming tracestate data, allow for third-party…

3d1f800

… data (#3275)

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

feat(tracing): Handle incoming tracestate data, allow for third-party…

ec8460f

… data (#3275)

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

feat(tracing): Handle incoming tracestate data, allow for third-party…

9560b13

… data (#3275)

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): Handle incoming tracestate data, allow for third-party data #3275

feat(tracing): Handle incoming tracestate data, allow for third-party data #3275

Uh oh!

lobsterkatie commented Feb 23, 2021 •

edited

Loading

Uh oh!

github-actions bot commented Feb 23, 2021 •

edited

Loading

Uh oh!

lobsterkatie commented Feb 24, 2021

Uh oh!

HazAT left a comment

Uh oh!

Uh oh!

kamilogorek Feb 26, 2021 •

edited

Loading

Uh oh!

lobsterkatie Feb 26, 2021

Uh oh!

Uh oh!

kamilogorek Feb 26, 2021

Uh oh!

lobsterkatie Feb 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	[before, after] = header.split(sentryEntry).map(s => s.replace(/^,\|,$/g, ''));
	[before, after] = header.split(sentryEntry).map(s => s.replace(/,/g, ''));

Uh oh!

feat(tracing): Handle incoming tracestate data, allow for third-party data #3275

feat(tracing): Handle incoming tracestate data, allow for third-party data #3275

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

size-limit report

Uh oh!

lobsterkatie commented Feb 24, 2021

Uh oh!

HazAT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamilogorek Feb 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!

lobsterkatie Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamilogorek Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lobsterkatie commented Feb 23, 2021 •

edited

Loading

github-actions bot commented Feb 23, 2021 •

edited

Loading

kamilogorek Feb 26, 2021 •

edited

Loading