-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
7bba4ca
f6a59b1
64c9935
7a8608c
4f7d68a
ee8447a
365e8a9
9831bec
d1a348d
248a9b6
3074bc5
5da55e6
7a85889
b0b65c7
a1811ce
e4b7631
906a116
97a13b9
b86bdb7
335a646
dd5fe3f
8028d9e
27a7afc
126e3b1
0e3c3d3
fbd7226
969567d
28b7789
32a9fc0
2d339ba
a76c891
9cf8644
9d4b623
3d953ad
08c708a
ed68239
fafc4af
5580981
7fba2fb
18df790
55aa327
bf06b7b
File filter
Filter by extension
Conversations
Jump to
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); | ||
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. 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, | ||
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. 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? 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. 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.