presets(vercel): export tracing channels messages as OTLP spans#4355
presets(vercel): export tracing channels messages as OTLP spans#4355RihanArfan wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a Vercel-specific Nitro runtime telemetry plugin that instruments ChangesVercel Runtime Telemetry Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
commit: |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
d329be1 to
4e220cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/presets/vercel/runtime/telemetry.ts (2)
221-225: ⚡ Quick winEmit warnings in recoverable catch blocks instead of fully silent fallback.
Line 223 and Line 347 currently discard error context entirely. Keeping fallback behavior is fine, but add a warning with channel/phase so telemetry regressions are diagnosable.
Proposed fix
if (describe) { try { return describe(channel, data); - } catch { - // Fall through to the generic span below. + } catch (error) { + console.warn("[vercel-telemetry] span describer failed", { channel, error }); } } @@ asyncEnd(message) { try { const start = starts.get(message); if (start === undefined) return; starts.delete(message); reportSpan(describeSpan(name, message), start, message.error); - } catch { - // Telemetry must never break the traced operation. + } catch (error) { + console.warn("[vercel-telemetry] asyncEnd processing failed", { + channel: name, + error, + }); } },As per coding guidelines: "Use warnings for recoverable situations; throw for invalid states".
Also applies to: 347-349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/vercel/runtime/telemetry.ts` around lines 221 - 225, In the catch block for the describe function call where channel and data are used, add a warning emission that includes the channel identifier and phase information instead of silently falling through. This allows telemetry issues to be diagnosed when the describe call fails. The same pattern should be applied to the similar catch block mentioned at line 347-349 that also silently discards error context. Ensure both recoverable error scenarios emit diagnostic warnings with relevant context while maintaining the fallback behavior.Source: Coding guidelines
1-406: 🏗️ Heavy liftSplit this runtime module; it exceeds the >200 LoC guideline.
This file currently mixes distinct concerns (span model, describers, buffering/export, diagnostics patching). Please split into focused runtime modules (for example
_span.ts,_describers.ts,_exporter.ts) and keep the plugin entry thin.As per coding guidelines: "Split logic across files; avoid long single-file modules (>200 LoC). Use
_*prefix for internal files".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/vercel/runtime/telemetry.ts` around lines 1 - 406, The telemetry.ts file currently contains over 400 lines mixing span model, channel describers, buffering/export logic, and diagnostics patching. Split this into focused internal modules: create `_span.ts` containing the `Span` class and span-related utilities like `Span.nowUnixNano()` and `Span.randomSpanId()`; create `_describers.ts` containing the channel describer functions (`describeH3Request`, `describeSrvxRequest`, `describeSrvxMiddleware`, `describeUnstorage`) and their helper types and constants (`CHANNEL_DESCRIBERS`, `PREFIX_DESCRIBERS`, `describeSpan`); create `_exporter.ts` containing the buffering and export logic (`pendingSpans`, `reportSpan`, `envelope`); and keep the main telemetry.ts file thin with just the plugin export definition and the diagnostics patching logic. Move helper utilities (`attr`, `asRecord`, `asString`, `label`, `errorMessage`, `exceptionEvent`) to the appropriate module where they are primarily used. Import the split modules back into the main file to maintain the current behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/presets/vercel/runtime/telemetry.ts`:
- Around line 157-161: The `sink.reportSpans` call within the
`context.waitUntil` async callback is not protected from throwing errors, which
can cause promise rejection and leak telemetry failures into request teardown
behavior. Wrap the `sink.reportSpans(envelope(batch))` call in a try-catch block
to handle any potential errors gracefully, catching exceptions and logging them
as warnings rather than allowing the error to propagate and reject the waitUntil
promise.
---
Nitpick comments:
In `@src/presets/vercel/runtime/telemetry.ts`:
- Around line 221-225: In the catch block for the describe function call where
channel and data are used, add a warning emission that includes the channel
identifier and phase information instead of silently falling through. This
allows telemetry issues to be diagnosed when the describe call fails. The same
pattern should be applied to the similar catch block mentioned at line 347-349
that also silently discards error context. Ensure both recoverable error
scenarios emit diagnostic warnings with relevant context while maintaining the
fallback behavior.
- Around line 1-406: The telemetry.ts file currently contains over 400 lines
mixing span model, channel describers, buffering/export logic, and diagnostics
patching. Split this into focused internal modules: create `_span.ts` containing
the `Span` class and span-related utilities like `Span.nowUnixNano()` and
`Span.randomSpanId()`; create `_describers.ts` containing the channel describer
functions (`describeH3Request`, `describeSrvxRequest`, `describeSrvxMiddleware`,
`describeUnstorage`) and their helper types and constants (`CHANNEL_DESCRIBERS`,
`PREFIX_DESCRIBERS`, `describeSpan`); create `_exporter.ts` containing the
buffering and export logic (`pendingSpans`, `reportSpan`, `envelope`); and keep
the main telemetry.ts file thin with just the plugin export definition and the
diagnostics patching logic. Move helper utilities (`attr`, `asRecord`,
`asString`, `label`, `errorMessage`, `exceptionEvent`) to the appropriate module
where they are primarily used. Import the split modules back into the main file
to maintain the current behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba5022c7-a3ad-432e-8208-e5e97ced9d45
📒 Files selected for processing (3)
src/presets/vercel/preset.tssrc/presets/vercel/runtime/telemetry-types.tssrc/presets/vercel/runtime/telemetry.ts
| context.waitUntil(async () => { | ||
| const batch = pendingSpans.get(traceId); | ||
| pendingSpans.delete(traceId); | ||
| if (batch && batch.length > 0) sink.reportSpans(envelope(batch)); | ||
| }); |
There was a problem hiding this comment.
Guard the waitUntil flush from reportSpans throws.
On Line 160, sink.reportSpans(...) is not protected. If it throws, the async waitUntil task rejects and telemetry failures can leak into request teardown behavior.
Proposed fix
context.waitUntil(async () => {
const batch = pendingSpans.get(traceId);
pendingSpans.delete(traceId);
- if (batch && batch.length > 0) sink.reportSpans(envelope(batch));
+ if (!batch || batch.length === 0) return;
+ try {
+ sink.reportSpans(envelope(batch));
+ } catch (error) {
+ console.warn("[vercel-telemetry] reportSpans failed", {
+ traceId,
+ spanCount: batch.length,
+ error,
+ });
+ }
});As per coding guidelines: "Prefer explicit errors over silent failures in error handling" and "Use warnings for recoverable situations; throw for invalid states".
📝 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.
| context.waitUntil(async () => { | |
| const batch = pendingSpans.get(traceId); | |
| pendingSpans.delete(traceId); | |
| if (batch && batch.length > 0) sink.reportSpans(envelope(batch)); | |
| }); | |
| context.waitUntil(async () => { | |
| const batch = pendingSpans.get(traceId); | |
| pendingSpans.delete(traceId); | |
| if (!batch || batch.length === 0) return; | |
| try { | |
| sink.reportSpans(envelope(batch)); | |
| } catch (error) { | |
| console.warn("[vercel-telemetry] reportSpans failed", { | |
| traceId, | |
| spanCount: batch.length, | |
| error, | |
| }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/presets/vercel/runtime/telemetry.ts` around lines 157 - 161, The
`sink.reportSpans` call within the `context.waitUntil` async callback is not
protected from throwing errors, which can cause promise rejection and leak
telemetry failures into request teardown behavior. Wrap the
`sink.reportSpans(envelope(batch))` call in a try-catch block to handle any
potential errors gracefully, catching exceptions and logging them as warnings
rather than allowing the error to propagate and reject the waitUntil promise.
Source: Coding guidelines
🔗 Linked issue
#4001
❓ Type of change
📚 Description
Export tracing channel messages as OpenTelemetry spans when deployed on Vercel. This adds a Vercel-preset runtime plugin that turns those events into OpenTelemetry spans and reports them to Vercel's tracing pipeline, so app-level spans show up in Vercel session traces with no OpenTelemetry SDK bundled.
See https://vercel.com/docs/tracing/session-tracing for Vercel usage
📝 Checklist
POC notes: