feat(tracing): Handle incoming tracestate data, allow for third-party data by lobsterkatie · Pull Request #3275 · getsentry/sentry-javascript

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

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

+ 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.

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

+
+ // 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.

… 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 b808fccCompare 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 fbd3c0fCompare 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 06e8d3bCompare 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

Comment threadpackages/serverless/src/awslambda.ts

kamilogorek reviewed Feb 26, 2021
View reviewed changes

Comment threadpackages/tracing/src/utils.ts Outdated

kamilogorek reviewed Feb 26, 2021
View reviewed changes

Comment threadpackages/tracing/src/utils.ts Outdated

kamilogorek reviewed Feb 26, 2021
View reviewed changes

Comment threadpackages/tracing/src/utils.ts Outdated

apply code review fixes
e5cea2e

lobsterkatie merged commit 4cf7262 into kmclb-dynamic-sampling-data-in-envelopesFeb 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

Navigation Menu

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
lobsterkatie merged 13 commits into
kmclb-dynamic-sampling-data-in-envelopesfrom
kmclb-handle-third-party-tracestate

lobsterkatie commented Feb 23, 2021•
edited
Loading

Uh oh!

github-actionsBot commented Feb 23, 2021•
edited
Loading

Uh oh!

lobsterkatie commented Feb 24, 2021

Uh oh!

HazAT left a comment

Uh oh!

Uh oh!

kamilogorekFeb 26, 2021•
edited
Loading

Uh oh!

lobsterkatieFeb 26, 2021

Uh oh!

Uh oh!

kamilogorekFeb 26, 2021

Uh oh!

lobsterkatieFeb 26, 2021

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

Uh oh!

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-actionsBot 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!

kamilogorekFeb 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!

lobsterkatieFeb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamilogorekFeb 26, 2021

Choose a reason for hiding this comment

Uh oh!

lobsterkatieFeb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lobsterkatie commented Feb 23, 2021•
edited
Loading

github-actionsBot commented Feb 23, 2021•
edited
Loading

kamilogorekFeb 26, 2021•
edited
Loading