Add `tracestate` header to outgoing requests #3092

lobsterkatie · 2020-12-02T06:45:52Z

This adds the tracestate header to outgoing requests in both browser (XHR and fetch) and node (http).

Major components:

A new tracestate property on the Transaction class
- Either passed in the transactionContext or computed when the transaction is created
- Equal to a JSONified, base64-encoded version of an object containing trace_id, release, environment, and public_key, with any = padding on the end removed
- Used both to provide a header value for outgoing requests and an envelope header value for transaction events
Inclusion of a tracestate header on all outgoing requests
Use of the already-calculated value for trace envelope header
Tests for all of the above

Reading and using incoming tracestate header values (including accounting for other vendors' data which might also be present, as well as multiple tracestate headers) will be handled in a later PR.

github-actions · 2020-12-02T06:58:34Z

HUMAN EDIT: Do not panic. These numbers are wrong for some reason. See #3092 (comment).

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	26.3 KB (+24.7% 🔺)
@sentry/browser - Webpack	27.32 KB (+24.35% 🔺)
@sentry/react - Webpack	27.32 KB (+24.35% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	33.72 KB (+20.12% 🔺)

HazAT

This concerns me a lot 😬
Not for merging this into the test branch but this would mean it's a hard blocker from releasing it into public.

But good to merge in the branch

lobsterkatie · 2020-12-15T16:30:36Z

This concerns me a lot 😬
Not for merging this into the test branch but this would mean it's a hard blocker from releasing it into public.

But good to merge in the branch

I asked Kamil about this - the numbers are wrong.

Master:

This branch:

RaduW

Really nice.

The only thing I don't like is tracestate in Event.

Also it would be a bit easier if you kept code shuffles that do not affect (as far as I can figure out) the current PR into a separate PR ( I'm referring at compat.ts and misc.ts).

RaduW · 2020-12-15T17:28:37Z

packages/core/src/request.ts

+  return {
+    body: JSON.stringify(event),
+    type: event.type || 'event',
+    url: api.getStoreEndpointWithUrlEncodedAuth(),
+  };


Nit: Not directly related to the PR but, Since we are now using envelopes for both transactions and sessions it would be good to have it also for events (so everything goes to the envelope endpoint).

Yeah, I'm not sure why we don't. I have no opposition to doing so, but I presume error events were separated out for a reason. Perhaps @HazAT or @rhcarvalho would have more context here.

In any case, as you say - a question for another day.

Moving error events over to the new endpoint break compatibility for people running their own sentry server (with a version prior to the introduction of envelopes).
Future SDK versions might change this, likely on a new major version bump.

RaduW · 2020-12-15T17:30:44Z

packages/core/src/request.ts

+  const req: SentryRequest = {
+    // The trailing newline is optional; leave it off to avoid sending unnecessary bytes.
+    // body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(event)\n}`,
+    body: `${envelopeHeaders}\n${itemHeaders}\n${JSON.stringify(event)}`,


Nit: having a function (or an object) to build envelopes would be nicer (and a little bit more 'typed').

Again, don't necessarily disagree. That code predated this PR, which just pulled it out into a separate method. What I take from the comments there, though, is that we're trying to keep the implementation as lightweight as possible, (presumably in aid of our never-ending quest to shrink our bundle size).

RaduW · 2020-12-15T17:45:50Z

packages/node/test/integrations/http.test.ts

+  it("doesn't attach tracing headers to outgoing sentry requests", () => {
    nock('http://squirrelchasers.ingest.sentry.io')
      .get('/api/12312012/store/')
      .reply(200);


We'll probably need send tracing headers in the near future, especially if we continue to send events to the store endpoint (as opposed to the envelope endpoint).

Will error events be subject to the same dynamic sampling, based on the same trace context, as transaction events?

packages/tracing/src/span.ts

packages/tracing/src/transaction.ts

packages/types/src/event.ts

lobsterkatie · 2020-12-16T04:06:29Z

The only thing I don't like is tracestate in Event.

Also it would be a bit easier if you kept code shuffles that do not affect (as far as I can figure out) the current PR into a separate PR ( I'm referring at compat.ts and misc.ts).

Both fixed.

github-actions · 2020-12-16T04:16:28Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	26.87 KB (+24.27% 🔺)
@sentry/browser - Webpack	27.79 KB (+0.53% 🔺)
@sentry/react - Webpack	27.82 KB (+0.52% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	34.14 KB (+19.83% 🔺)

packages/types/src/event.ts

… header

lobsterkatie · 2021-02-17T00:29:02Z

Going to merge this into the feature branch, just to be able to stick a pin somewhere.

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-correlation-context-in-header branch 2 times, most recently from 9803596 to a28a330 Compare December 8, 2020 00:49

lobsterkatie force-pushed the kmclb-dynamic-sampling-data-in-envelopes branch from 6255084 to 1d5691e Compare December 8, 2020 00:55

lobsterkatie force-pushed the kmclb-correlation-context-in-header branch 4 times, most recently from 782a8ed to ee52cbd Compare December 11, 2020 08:23

lobsterkatie marked this pull request as ready for review December 11, 2020 08:28

lobsterkatie requested a review from kamilogorek as a code owner December 11, 2020 08:28

lobsterkatie changed the title ~~[WIP] Add tracestate header to outgoing requests~~ Add tracestate header to outgoing requests Dec 11, 2020

lobsterkatie force-pushed the kmclb-correlation-context-in-header branch 2 times, most recently from 2339b90 to 4af65a8 Compare December 14, 2020 19:23

lobsterkatie force-pushed the kmclb-dynamic-sampling-data-in-envelopes branch from 1d5691e to b80f797 Compare December 14, 2020 19:29

lobsterkatie requested review from HazAT, jan-auer and RaduW December 14, 2020 21:13

HazAT reviewed Dec 15, 2020

View reviewed changes

RaduW approved these changes Dec 15, 2020

View reviewed changes

lobsterkatie force-pushed the kmclb-dynamic-sampling-data-in-envelopes branch from 92729aa to b240401 Compare December 16, 2020 04:01

lobsterkatie force-pushed the kmclb-correlation-context-in-header branch from 4af65a8 to 76b8b91 Compare December 16, 2020 04:06

RaduW reviewed Dec 16, 2020

View reviewed changes

packages/types/src/event.ts Outdated Show resolved Hide resolved

lobsterkatie force-pushed the kmclb-dynamic-sampling-data-in-envelopes branch from b240401 to 41e1467 Compare December 16, 2020 15:56

lobsterkatie force-pushed the kmclb-correlation-context-in-header branch from 76b8b91 to 3ee4735 Compare December 16, 2020 17:29

lobsterkatie force-pushed the kmclb-dynamic-sampling-data-in-envelopes branch from 41e1467 to 45ab20c Compare February 6, 2021 20:59

lobsterkatie mentioned this pull request Feb 6, 2021

ref(tracing): Prework for initial tracestate implementation #3242

Merged

add tracestate property to Transaction class

18c2393

lobsterkatie added 4 commits February 7, 2021 08:39

add method to generate outgoing trace headers

e16cd35

attach both headers to outgoing requests instead of just sentry-trace…

003ab3f

… header

include transaction's tracestate in corresponding event's debug_meta

cfd046f

use encoded value for tracestate in envelope header

30c98f8

lobsterkatie force-pushed the kmclb-correlation-context-in-header branch from 3ee4735 to 30c98f8 Compare February 7, 2021 17:43

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

lobsterkatie deleted the kmclb-correlation-context-in-header branch February 17, 2021 00:29

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

Add tracestate header to outgoing requests (#3092)

3a11bbb

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

000f554

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

afabf4e

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

0a4976c

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

2958323

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

b8b76b9

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

feat(tracing): Add tracestate header to outgoing requests (#3092)

192e4ca

lobsterkatie mentioned this pull request Aug 20, 2021

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

Merged

Uh oh!

Add tracestate header to outgoing requests #3092

Add tracestate header to outgoing requests #3092

Uh oh!

Conversation

lobsterkatie commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 2, 2020 • edited by lobsterkatie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

HazAT left a comment

Choose a reason for hiding this comment

Uh oh!

lobsterkatie commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RaduW left a comment

Choose a reason for hiding this comment

Uh oh!

RaduW Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

rhcarvalho Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

RaduW Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

RaduW Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

lobsterkatie Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lobsterkatie commented Dec 16, 2020

Uh oh!

github-actions bot commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

Uh oh!

lobsterkatie commented Feb 17, 2021

Uh oh!

Uh oh!

Add `tracestate` header to outgoing requests #3092

Add `tracestate` header to outgoing requests #3092

lobsterkatie commented Dec 2, 2020 •

edited

Loading

github-actions bot commented Dec 2, 2020 •

edited by lobsterkatie

Loading

lobsterkatie commented Dec 15, 2020 •

edited

Loading

github-actions bot commented Dec 16, 2020 •

edited

Loading