Uh oh!
There was an error while loading. Please reload this page.
feat(remix): Add orchestrion-based remix instrumentation - #22244
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
size-limit report 📦
|
Uh oh!
There was an error while loading. Please reload this page.
41d5562 to
24c9db3CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
929eaf5 to
e524832CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
RemixInstrumentation to orchestrionThere was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7957bd3. Configure here.
| // Additional things that need to be bundled but are not covered by the above | ||
| // Remix needs to bundle this so @remix-run/server-runtime is _also_ bundled | ||
| '@remix-run/node', | ||
| ]; |
There was a problem hiding this comment.
Global Remix force-bundle leak
Low Severity
instrumentedModuleNames always appends @remix-run/node for every orchestrion consumer, not only Remix apps. That helper feeds Vite ssr.noExternal and webpack/esbuild external stripping globally, so a Remix-specific bundling requirement leaks into Nest, Express, and other SDK setups.
Reviewed by Cursor Bugbot for commit 7957bd3. Configure here.
There was a problem hiding this comment.
this is fine, same with all others, if this is not found it will simply be ignored
| const MATCH_ROUTE_ID = 'match.route.id'; | ||
| const MATCH_PARAMS = 'match.params'; |
There was a problem hiding this comment.
Those attributes should probably also be added to sentry conventions (as deprecated) as we can use http.route and our url.path.parameter.<key instead.
There was a problem hiding this comment.
yes, totally. I wanted to leave this to a follow up task - imho once we are done it makes sense to go look for any attribute we set that is not imported from semantic attribtues and bulk-add them!
Uh oh!
There was an error while loading. Please reload this page.
`instrumentRemix` only subscribed to `callRouteAction` when `actionFormDataAttributes` was configured, so ACTION spans were never created unless form-data capture was opted into. The OTel instrumentation always patched `callRouteAction` and used those keys only for optional attribute extraction. Always subscribe; the request clone + async form-data read now happen only when capture is configured. Adds a regression test (separate file so the channel subscribes with no form-data config) asserting ACTION spans are still created. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The OpenTelemetry branch of `remixIntegration` passed the form-data key map
straight into `instrumentRemixWithOpenTelemetry`, which expects
`{ actionFormDataAttributes: ... }`. As a result the map was treated as the
whole instrumentation config, so `RemixInstrumentation`'s default
`{ _action: 'actionType' }` mapping stayed in place — applying even when capture
wasn't configured, and ignoring the opted-in keys when it was.
Wrap the map in the options object. Adds a test for the OTel branch asserting the
call shape (both opted-in and not).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>The build-injection guard only asserted the `requestHandler` and `callRouteLoader` publishers, but the transform config also injects `matchServerRoutes` and `callRouteAction`. A silent failure to inject either of those would have slipped past the guard, whose whole purpose is to prove build-time injection. Assert all four channels. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`deferSpanEnd` read the form data only at `asyncEnd`, after `callRouteAction` had already returned. A channel can't delay the action promise the way the vendored instrumentation did, so the parent `requestHandler` span could finish and flush the transaction before the form-data attributes were applied and the ACTION span ended, dropping that data. Start the form-data read at span start (from a clone taken before the action consumes the body) so it overlaps the action's execution. By `asyncEnd` the read is virtually always resolved, so applying the attributes and ending the span costs a single microtask - comfortably inside the response-rendering gap before the parent span ends. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
7957bd3 to
cfbb502Compare| // Additional things that need to be bundled but are not covered by the above | ||
| // Remix needs to bundle this so @remix-run/server-runtime is _also_ bundled | ||
| '@remix-run/node', |
There was a problem hiding this comment.
l: I know this will be ignored if unused, but I'd vote for moving this into the vite plugin for remix once we have either a orchestrion opt-in flag or latest when we enable orchestrion by default
There was a problem hiding this comment.
yes that makes sense I guess! 👍
JPeer264
left a comment
There was a problem hiding this comment.
Not a Remix pro, but implementation LGTM
Uh oh!
There was an error while loading. Please reload this page.
Rewrites the vendored
RemixInstrumentation(an OpenTelemetryInstrumentationBasethat patched@remix-run/server-runtime) to a diagnostics-channel listener, with orchestrion injecting the channels into the instrumented module at build time.SDK changes
@sentry/server-utils— populated the orchestrion config for@remix-run/server-runtime(orchestrion/config/remix.ts)@sentry/remix— adjusted theremixIntegrationso it runs either OTEL-based instrumentation, or prchestrion-based instrumentation if opted in.E2E changes
remix-orchestriontest app intocreate-remix-app-v2as anINJECT_ORCHESTRION-gatedsentryTestvariant, so one codebase runs both the OpenTelemetry and orchestrion paths (build-time injection via the orchestrion Vite plugin, DB routes + docker for mysql/ioredis, and a build-injection assertion). The base transaction tests run in both variants as a parity check.@remix-run/server-runtimereachable by the build-time transform,@remix-run/nodeis force-bundled (it re-exports server-runtime, which otherwise stays external and is loaded byremix-serveat runtime, outside the Vite bundle).Fixes#20910