Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
ref: Make dynamic sampling context mutable#5710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
7bba4caf6a59b164c99357a8608c4f7d68aee8447a365e8a99831becd1a348d248a9b63074bc55da55e67a85889b0b65c7a1811cee4b7631906a11697a13b9b86bdb7335a646dd5fe3f8028d9e27a7afc126e3b10e3c3d3fbd7226969567d28b778932a9fc02d339baa76c8919cf86449d4b6233d953ad08c708aed68239fafc4af55809817fba2fb18df79055aa327bf06b7bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { hasTracingEnabled } from '@sentry/tracing'; | ||
| import { serializeBaggage } from '@sentry/utils'; | ||
| import { dynamicSamplingContextToSentryBaggageHeader } from '@sentry/utils'; | ||
| import { NextPage } from 'next'; | ||
| import { isBuild } from '../../utils/isBuild'; | ||
| @@ -42,7 +42,9 @@ export function withSentryServerSideGetInitialProps(origGetInitialProps: GetInit | ||
| const requestTransaction = getTransactionFromRequest(req!); | ||
| if (requestTransaction) { | ||
| initialProps._sentryTraceData = requestTransaction.toTraceparent(); | ||
| initialProps._sentryBaggage = serializeBaggage(requestTransaction.getBaggage()); | ||
| const dynamicSamplingContext = requestTransaction.getDynamicSamplingContext(); | ||
| initialProps._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is repeated 4x - can we extract into a helper func? Can do this in another PR. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We intentionally kept it this way for the ongoing Next.js changes and said we were gonna extract duplicate logic later, since we don't know yet if extracting it now will complicate things | ||
| } | ||
| return initialProps; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -2,7 +2,7 @@ import { captureException, getCurrentHub, startTransaction } from '@sentry/core' | ||
| import { addRequestDataToEvent } from '@sentry/node'; | ||
| import { getActiveTransaction } from '@sentry/tracing'; | ||
| import { Transaction } from '@sentry/types'; | ||
| import { extractTraceparentData, fill, isString, parseBaggageSetMutability } from '@sentry/utils'; | ||
| import { baggageHeaderToDynamicSamplingContext, extractTraceparentData, fill } from '@sentry/utils'; | ||
| import * as domain from 'domain'; | ||
| import { IncomingMessage, ServerResponse } from 'http'; | ||
| @@ -88,19 +88,19 @@ export function callTracedServerSideDataFetcher<F extends (...args: any[]) => Pr | ||
| if (requestTransaction === undefined) { | ||
| const sentryTraceHeader = req.headers['sentry-trace']; | ||
| const rawBaggageString = req.headers && isString(req.headers.baggage) && req.headers.baggage; | ||
| const rawBaggageString = req.headers && req.headers.baggage; | ||
| const traceparentData = | ||
| typeof sentryTraceHeader === 'string' ? extractTraceparentData(sentryTraceHeader) : undefined; | ||
| const baggage = parseBaggageSetMutability(rawBaggageString, traceparentData); | ||
| const dynamicSamplingContext = baggageHeaderToDynamicSamplingContext(rawBaggageString); | ||
| const newTransaction = startTransaction({ | ||
| op: 'nextjs.data.server', | ||
| name: options.requestedRouteName, | ||
| ...traceparentData, | ||
| metadata: { | ||
| source: 'route', | ||
| baggage, | ||
| dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we have an invalid We could either: I'm leaning toward b - so I would do something like so: ...(traceparentData&&dynamicSamplingContext&&{ dynamicSamplingContext }),We could also construct a helper for this, since we repeat this logic in some other places. ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason for this ternary here was to freeze baggage if there is a In normal circumstances, it shouldn't happen that there is a Do you have any other thoughts regarding this, otherwise I would leave it as is. Having a helper seems overkill to me for a simple if statement (even though I admit it is repeated in a lot of places). What I can do, is add a comment where we do this to explain that DSC needs to be frozen when there is trace parent data. Wdyt? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ahhhh I see - this makes sense. Let me try playing around with the helper to clean it up a little. Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I give up - I think we keep this for now then. | ||
| }, | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.