-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(nuxt): Add Cloudflare Nitro plugin #15597
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
Conversation
bc63ab4
to
fc62672
Compare
243b681
to
0800086
Compare
packages/cloudflare/src/request.ts
Outdated
* | ||
* @default false | ||
*/ | ||
continueTraceFromPropagationContext?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbhiPrasad Is this a good place for adding the option? As it's only needed in the requestHandler, I added it only here and not in the general Cloudflare SDK options.
Actually, I don't need this option because it would always take same trace ID from the Cloudflare environment which leads to multiple pageload spans within a trace as they all have the same trace ID in the meta tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quick drive by, I'll review in more detail tomorrow
if (traceData && Object.keys(traceData).length > 0) { | ||
// Storing trace data in the event context for later use in HTML meta-tags (enables correct connection of parent/child span relationships) | ||
// @ts-expect-error Storing a new key in the event context | ||
event.context[TRACE_DATA_KEY] = traceData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of attaching to event.context
, can we instead track this via a WeakMap
?
So we hold a reference to event.context
or event
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean using the event as a key?
That's a bit tricky as the event in localFetch
is a full Cloudflare event which A LOT of data. And the event in the render:html
hook is only a string (e.g. "[GET] /"
). So I cannot get the value out of the WeakMap anymore, as I don't have a key anymore. Or would you have an idea on how to do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that context.cf
is available in both and looks the same. Using a WeakMap now: a20ebef
❌ Unsupported file formatUpload processing failed due to unsupported file format. Please review the parser error message:
For more help, visit our troubleshooting guide. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Sentry Plugin Fails to Inject Trace Data
The sentryCloudflareNitroPlugin
fails to inject Sentry trace data into HTML meta tags. This is due to a WeakMap
key mismatch: trace data is stored using event.context.cf
from a CfEventType
in nitroApp.localFetch
, but the render:html
hook attempts retrieval using event.context.cf
from an H3Event
. The H3Event
's context.cf
is either not correctly provided by nitroApp
or is a different object instance, preventing the WeakMap
lookup from succeeding.
packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts#L89-L152
sentry-javascript/packages/nuxt/src/runtime/plugins/sentry-cloudflare.server.ts
Lines 89 to 152 in a20ebef
(nitroApp: NitroApp): void => { | |
const traceDataMap = new WeakMap<object, ReturnType<typeof getTraceData>>(); | |
nitroApp.localFetch = new Proxy(nitroApp.localFetch, { | |
async apply(handlerTarget, handlerThisArg, handlerArgs: [string, unknown]) { | |
setAsyncLocalStorageAsyncContextStrategy(); | |
const cloudflareOptions = typeof optionsOrFn === 'function' ? optionsOrFn(nitroApp) : optionsOrFn; | |
const pathname = handlerArgs[0]; | |
const event = handlerArgs[1]; | |
if (!isEventType(event)) { | |
logger.log("Nitro Cloudflare plugin did not detect a Cloudflare event type. Won't patch Cloudflare handler."); | |
return handlerTarget.apply(handlerThisArg, handlerArgs); | |
} else { | |
// Usually, the protocol already includes ":" | |
const url = `${event.protocol}${event.protocol.endsWith(':') ? '' : ':'}//${event.host}${pathname}`; | |
const request = new Request(url, { | |
method: event.method, | |
headers: event.headers, | |
cf: event.context.cf, | |
}) as Request<unknown, IncomingRequestCfProperties<unknown>>; | |
const requestHandlerOptions = { | |
options: cloudflareOptions, | |
request, | |
context: event.context.cloudflare.context, | |
}; | |
return wrapRequestHandler(requestHandlerOptions, () => { | |
const isolationScope = getIsolationScope(); | |
const newIsolationScope = | |
isolationScope === getDefaultIsolationScope() ? isolationScope.clone() : isolationScope; | |
const traceData = getTraceData(); | |
if (traceData && Object.keys(traceData).length > 0) { | |
// Storing trace data in the WeakMap using event.context.cf as key for later use in HTML meta-tags | |
traceDataMap.set(event.context.cf, traceData); | |
logger.log('Stored trace data for later use in HTML meta-tags: ', traceData); | |
} | |
logger.log( | |
`Patched Cloudflare handler (\`nitroApp.localFetch\`). ${ | |
isolationScope === newIsolationScope ? 'Using existing' : 'Created new' | |
} isolation scope.`, | |
); | |
return handlerTarget.apply(handlerThisArg, handlerArgs); | |
}); | |
} | |
}, | |
}); | |
// @ts-expect-error - 'render:html' is a valid hook name in the Nuxt context | |
nitroApp.hooks.hook('render:html', (html: NuxtRenderHTMLContext, { event }: { event: H3Event }) => { | |
const storedTraceData = event?.context?.cf ? traceDataMap.get(event.context.cf) : undefined; | |
if (storedTraceData && Object.keys(storedTraceData).length > 0) { | |
logger.log('Using stored trace data for HTML meta-tags: ', storedTraceData); | |
addSentryTracingMetaTags(html.head, storedTraceData); | |
} else { | |
addSentryTracingMetaTags(html.head); | |
} | |
}); |
Bug: Meta Tag Check Fails on Non-String Elements
The addSentryTracingMetaTags
function's duplicate meta tag check incorrectly assumes all head
array elements are strings, leading to runtime errors when tag.includes()
is called on non-string types. Furthermore, the string-based detection (tag.includes('meta') && tag.includes('sentry-trace')
) is fragile and can produce false positives, preventing legitimate meta tags from being added.
packages/nuxt/src/runtime/utils.ts#L46-L47
sentry-javascript/packages/nuxt/src/runtime/utils.ts
Lines 46 to 47 in a20ebef
if (head.some(tag => tag.includes('meta') && tag.includes('sentry-trace'))) { |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Addition to the Cursor comments:
|
return handlerTarget.apply(handlerThisArg, handlerArgs); | ||
} else { | ||
// Usually, the protocol already includes ":" | ||
const url = `${event.protocol}${event.protocol.endsWith(':') ? '' : ':'}//${event.host}${pathname}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it doesn't seem like we are using pathname
more than once, we may as well use the handler argument directly 😉
const url = `${event.protocol}${event.protocol.endsWith(':') ? '' : ':'}//${event.host}${pathname}`; | |
const url = `${event.protocol}${event.protocol.endsWith(':') ? '' : ':'}//${event.host}${handlerArgs[0]}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very valid but I added the variables on purpose so it's easier to read and understand :)
It's not very clear that this first array element is the pathname and with the variable it's more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally get you here.
It's especially relevant in case we want to use the path name in the future. Again I was just being nitpicky here 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
## DESCRIBE YOUR PR Docs for getsentry/sentry-javascript#15597 closes getsentry/sentry-javascript#16780 ## IS YOUR CHANGE URGENT? Help us prioritize incoming PRs by letting us know when the change needs to go live. - [ ] Urgent deadline (GA date, etc.): <!-- ENTER DATE HERE --> - [ ] Other deadline: <!-- ENTER DATE HERE --> - [ ] None: Not urgent, can wait up to 1 week+ ## SLA - Teamwork makes the dream work, so please add a reviewer to your PRs. - Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it. Thanks in advance for your help! ## PRE-MERGE CHECKLIST *Make sure you've checked the following before merging your changes:* - [ ] Checked Vercel preview for correctness, including links - [ ] PR was reviewed and approved by any necessary SMEs (subject matter experts) - [ ] PR was reviewed and approved by a member of the [Sentry docs team](https://github.com/orgs/getsentry/teams/docs)
A Nitro plugin which initializes Sentry when deployed to Cloudflare.
Add
@sentry/cloudflare
as additional dependency (same version as@sentry/nuxt
)Remove the previous server config file:
sentry.server.config.ts
Add a plugin in
server/plugins
(e.g.server/plugins/sentry-cloudflare-setup.ts
)Add this code in your plugin file
or with access to
nitroApp
: