fix(webapp): report full route path on Remix request spans#4309
fix(webapp): report full route path on Remix request spans#4309carderne wants to merge 1 commit into
Conversation
The @sentry/remix OpenTelemetry instrumentation sets each request span's name and http.route from only the matched leaf route's own path segment. For nested routes that made distinct pages collapse to one ambiguous segment (e.g. ":runParam"), so traces could not tell which route served a request. Reconstruct the full matched path by joining every segment in the match chain, falling back to the leaf path. Flat routes such as "api/v1/tasks/:taskId/trigger" are unchanged; nested routes now carry their full URL pattern. Applied to both the CJS and ESM builds via the existing pnpm patch.
|
WalkthroughRemix instrumentation now combines all matched nested route segments when setting 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9bd2bc-fd0b-4950-a754-2ead92238d21
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
patches/@sentry__remix@9.46.0.patch
📜 Review details
⏰ Context from checks skipped due to timeout. (32)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 12)
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 12)
- GitHub Check: sdk-compat / Node.js 20.20 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 12)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 12)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - npm)
- GitHub Check: sdk-compat / Node.js 24.18 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 22.23 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: sdk-compat / Node.js 26.4 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - pnpm)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp
- GitHub Check: typecheck / typecheck
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Build and publish previews
| + const routePath = | ||
| + (result || []) | ||
| + .map((match) => match?.route?.path) | ||
| + .filter((segment) => typeof segment === 'string' && segment.length > 0) | ||
| + .join('/') || route?.path; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Normalize multiple slashes in joined route paths.
When a root route segment (e.g., '/') is joined with child segments (e.g., 'users'), the resulting path will contain double slashes (e.g., '//users'). Normalizing the joined string prevents incorrectly formatted span names.
patches/@sentry__remix@9.46.0.patch#L13-L17: Append.replace(/\/\/+/g, '/')to the joined string in the CJS build to safely clean up the path.patches/@sentry__remix@9.46.0.patch#L68-L72: Append.replace(/\/\/+/g, '/')to the joined string in the ESM build to safely clean up the path.
💡 Proposed fixes
For the CJS build (patches/@sentry__remix@9.46.0.patch#L13-L17):
- const routePath =
- (result || [])
- .map((match) => match?.route?.path)
- .filter((segment) => typeof segment === 'string' && segment.length > 0)
- .join('/') || route?.path;
+ const routePath =
+ (result || [])
+ .map((match) => match?.route?.path)
+ .filter((segment) => typeof segment === 'string' && segment.length > 0)
+ .join('/')
+ .replace(/\/\/+/g, '/') || route?.path;For the ESM build (patches/@sentry__remix@9.46.0.patch#L68-L72):
- const routePath =
- (result || [])
- .map((match) => match?.route?.path)
- .filter((segment) => typeof segment === 'string' && segment.length > 0)
- .join('/') || route?.path;
+ const routePath =
+ (result || [])
+ .map((match) => match?.route?.path)
+ .filter((segment) => typeof segment === 'string' && segment.length > 0)
+ .join('/')
+ .replace(/\/\/+/g, '/') || route?.path;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + const routePath = | |
| + (result || []) | |
| + .map((match) => match?.route?.path) | |
| + .filter((segment) => typeof segment === 'string' && segment.length > 0) | |
| + .join('/') || route?.path; | |
| const routePath = | |
| (result || []) | |
| .map((match) => match?.route?.path) | |
| .filter((segment) => typeof segment === 'string' && segment.length > 0) | |
| .join('/') | |
| .replace(/\/\/+/g, '/') || route?.path; |
📍 Affects 1 file
patches/@sentry__remix@9.46.0.patch#L13-L17(this comment)patches/@sentry__remix@9.46.0.patch#L68-L72
| diff --git a/build/esm/vendor/instrumentation.js b/build/esm/vendor/instrumentation.js | ||
| index e33b84eecdffe3b10a5e50f13bafaf90775747c5..652a89c9194ea1d0bb0d748fb8f7f060ca2cf5ba 100644 | ||
| --- a/build/esm/vendor/instrumentation.js | ||
| +++ b/build/esm/vendor/instrumentation.js | ||
| @@ -138,7 +138,14 @@ class RemixInstrumentation extends InstrumentationBase { | ||
|
|
||
| const route = (result || []).slice(-1)[0]?.route; | ||
|
|
||
| - const routePath = route?.path; | ||
| + // Join every matched route segment so nested routes report their | ||
| + // full URL pattern on http.route (and the span name), not just the | ||
| + // leaf route's own path segment. | ||
| + const routePath = | ||
| + (result || []) | ||
| + .map((match) => match?.route?.path) | ||
| + .filter((segment) => typeof segment === 'string' && segment.length > 0) | ||
| + .join('/') || route?.path; | ||
| if (span && routePath) { | ||
| span.setAttribute(SemanticAttributes.HTTP_ROUTE, routePath); | ||
| span.updateName(`remix.request ${routePath}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing action processing changes for the ESM build.
The patch applies the nested-route joining logic to the ESM build but entirely omits the callRouteAction modifications that were successfully applied to the CJS build (lines 24-54). Consequently, ESM consumers will still experience request cloning and form-data parsing, which breaks the intended bundle consistency and could result in request-body-consumption bugs.
Please add the missing patch hunks targeting build/esm/vendor/instrumentation.js to remove params.request.clone() and the formData() span enrichment, matching the CJS changes.
1 participant
Summary
Traces from the webapp's Remix server can't always tell which route served a request. The
@sentry/remixOpenTelemetry instrumentation sets a request span's name andhttp.routefrom only the matched leaf route's own path segment, so nested routes collapse to an ambiguous segment (e.g.:runParam) and several distinct pages share one opaque label.Fix
Reconstruct the full matched path by joining every segment in the match chain, falling back to the leaf path when the chain is empty. Flat routes such as
api/v1/tasks/:taskId/triggerare unchanged; nested routes now report their full URL pattern (e.g.orgs/:organizationSlug/projects/:projectParam/env/:envParam/runs/:runParam). Applied to both the CJS and ESM builds through the existing@sentry/remixpnpm patch.