Uh oh!
There was an error while loading. Please reload this page.
feat(remix): Migrate to opentelemetry-instrumentation-remix. - #12110
Conversation
81e86d3 to
8941895CompareUh oh!
There was an error while loading. Please reload this page.
e5d54a3 to
fb620b8CompareUh oh!
There was an error while loading. Please reload this page.
| return ( | ||
| transactionEvent.type === 'transaction' && | ||
| transactionEvent.contexts?.trace?.op === 'http.server' && | ||
| transactionEvent.contexts?.trace?.op === 'http' && |
There was a problem hiding this comment.
hmm, we should fix this to be http.server - this is important I believe!
There was a problem hiding this comment.
Marked requestHandler spans as http.server 👍
| */ | ||
| export function getDefaultIntegrations(options: RemixOptions): Integration[] { | ||
| return [ | ||
| ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), |
There was a problem hiding this comment.
Hmm, do we still instrument outgoing requests without this? Do we really need to remove this? Most otel instrumentation is on top of this...?
There was a problem hiding this comment.
Http instrumentation's functionality is covered by requestHandler spans by the opentelemetry-instrumentation-remix.
In this context, HTTP instrumentation also ends up setting an unparameterised route as the root transaction name as I've seen from the tests.
There was a problem hiding this comment.
Does the http span become the parent of the remix request handler spans?
If so, could we just extend the HTTP integration for remix to filter out server spans by default? So we still get instrumentation for outgoing requests?
Otherwise I wonder if we should actually vendor in https://github.com/justindsmith/opentelemetry-instrumentations-js/blob/main/packages/instrumentation-remix/src/instrumentation.ts and change it that way?
There was a problem hiding this comment.
Yes, makes sense to me, just extended the Http integration the same way it's done in Next.js. Also marked requestHandler spans as http.server.
Uh oh!
There was an error while loading. Please reload this page.
| }, | ||
| { | ||
| command: `PORT=${port} pnpm start`, | ||
| command: `PORT=${port} NODE_OPTIONS='--require ./instrument.server.cjs' pnpm start`, |
There was a problem hiding this comment.
Is it necessary to run it like this, does it not work with require on top? Just to clarify!
There was a problem hiding this comment.
Yes, we need to run it like this. esbuild does not bundle the instrumentation inside the entry files correctly.
There was a problem hiding this comment.
hmmm I wonder if this qualifies as a breaking change 😬 thoughts cc @AbhiPrasad ?
There was a problem hiding this comment.
I think this does qualify as a breaking change 😬
I guess we have to opt-in to this mode, we can't break the people who already put the work in to upgrade their sdk to v8.
There was a problem hiding this comment.
We can make this the default during onboarding though.
There was a problem hiding this comment.
I think that makes sense. So I would say we do:
- Expose
remixIntegrationbut do not add it by default - Expose
remixHttpIntegrationbut do not add it by default - Users have to add both of these if they want to opt-in (the latter "overwrites" the default http integration)
Does that sound good? 🤔
Questions that remain:
a. Can we find a way to make 2 obsolete? Can't think of a good way to do it sadly 😬
b. Should we then (eventually) document both paths, or just the "new" path? I guess the "old" way could remain as an alternative installation method...?
There was a problem hiding this comment.
Can we try adding another init option like useOtel to switch to this mode, and initialise the integrations without (or also with) exposing them?
This change also doesn't affect Express servers (or I guess any other custom server), a top-level import on the server file works in those cases. So we can still get rid of the Express adapter instrumentation, and keep the core instrumentation as an alternative for built-in server users (which I also guess is not very popular among Remix users looking at our issue reports)
I have updated the e2e tests using Express custom servers. (0e2f73a)
There was a problem hiding this comment.
Yes, that sounds good - let's make an option for this and adjust default integrations based on this!
3ec1138 to
06a185eCompare06a185e to
0e2f73aCompareAbhiPrasad
commented
May 31, 2024
@onurtemizkan could you add the new setup instructions to the readme of this PR? and also add a short note about what the migration looks like? |
| export function getDefaultIntegrations(options: RemixOptions): Integration[] { | ||
| return [ | ||
| ...getDefaultNodeIntegrations(options).filter(integration => integration.name !== 'Http'), | ||
| httpIntegration(), |
There was a problem hiding this comment.
Let's refactor this to something like:
functiongetRemixDefaultIntegrations(){return[...]}// further down:if(options.autoInstrumentRemix){options.defaultIntegrations=getRemixDefaultIntegrations(options);}else{instrumentServer();}This way users can opt in to the new behavior, but keep the old behavior working if needed?
This also means that wrapExpressCreateRequestHandler should probably remain as it was before, users need to opt-in to not use that anymore...?
There was a problem hiding this comment.
Skipping incoming requests worked well both on legacy and otel implementations. I think I managed to keep the old behaviour, without requiring wrapExpressCreateRequestHandler, creating http.server span manually.
Dropping a sample event here
| name: 'Remix', | ||
| setupOnce() { | ||
| addOpenTelemetryInstrumentation( | ||
| new RemixInstrumentation({ |
There was a problem hiding this comment.
m: Let's re-write this based on the changes done in #12213, which means extracting this into a instrumentRemix method which is called in setupOnce. You'll have to put the options into module scope so they can be updated later...!
39d7148 to
cd0d656Comparesize-limit report 📦
|
0bd6f5e to
8c9330aComparef61216b to
b44dd02Compare
Ref: #11040
Migrates Remix server-side SDK to opentelemetry-instrumentation-remix.
This PR also keeps the original implementation not the break the developer experience for non-Express Remix projects.
Remix projects using Express are supported as is using the new
autoInstrumentRemixoption.Usage with Express:
Usage with built-in Remix server:
You need to run the Remix server with
NODE_OPTIONS=--require(...)` set.This PR removes:
Express server adapter.
There is no need to use
wrapExpressCreateRequestHandleranymore even if you don't opt-in toautoInstrumentRemix.wrapExpressCreateRequestHandleris kept exported as a no-op function.Built in HTTP incoming request instrumentation for both
legacyandotelimplementations. Instead we markrequestHandlerspans as the roothttp.serverspans.When
autoInstrumentRemixis set totrue, this update replaces:Performance tracing on
action/loader/documentRequestfunctions. Leaving them to be traced byopentelemetry-instrumentation-remixRequest handler instrumentation as they are also traced by
opentelemetry-instrumentation-remixAuto-instrumentation for http as default integration for Remix SDK.
With the new instrumentation,
pageloadspan is the child of theloaderspan. (Example: Trace)Legacy instrumentation keeps recording
pageloadspan as the child of thehttp.serverspan. (Example: Trace)Also:
Migrates Remix integration tests from Jest to Vitest.
Fixes Backlogged Issues: