fix(tracing): close four silent gaps in Phoenix span coverage - #263
Open
NickB03 wants to merge 9 commits into
Open
fix(tracing): close four silent gaps in Phoenix span coverage#263NickB03 wants to merge 9 commits into
NickB03 wants to merge 9 commits into
Conversation
onFinish returned early on abort, before flushTraces(), so every aborted chat's spans died unexported when the serverless function terminated. Persistence still skips aborted/empty responses; only the flush is now unconditional. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ally Commit 1e8d68e moved flushTraces() into a finally block so aborted streams still export their OTel spans, but no test drove onFinish with isAborted or a missing responseMessage — the exact branch the fix exists for. Parameterize the ai mock's onFinish payload and add coverage for the abort path, the missing-responseMessage path, and persistStreamResults throwing, asserting flushTraces still runs and persistStreamResults is skipped/isolated as expected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing read these vars in this setup, so operators could set them, see no error, and still ship prompts to Phoenix. Routes them through a telemetryRecordingOptions() helper spread into every experimental_telemetry block, prunes the unhonoured vars from the schema, and corrects the docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ifier Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…M calls Both ran untraced, so their latency and failures never reached Phoenix — including the trending-suggestions call on the Vercel cron, where there is no UI to notice a failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…visible phoenix: 'ok' only proves the collector is reachable, not that this process registered an exporter. The HTTPS guard could silently disable tracing in production with the health check still green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…field Adds regression coverage for the globalThis.__polymorphTracingState assignments in instrumentation.ts (disabled-off, disabled-https) and for how /api/health surfaces that global as body.tracing, so a future refactor that reorders the default assignment or breaks the field gating is caught instead of silently reintroducing the blind-deploy visibility gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
STREAMING-OPERATIONS.md said onFinish skipped trace flushing for aborted responses; it now flushes unconditionally and only persistence is skipped. ENVIRONMENT-OPERATIONS.md's instrumentation.ts line reference was stale. API-AUXILIARY-ENDPOINTS.md did not document the new /api/health `tracing` field at all. Split out of a combined docs commit on the working branch so this PR carries only the tracing surface; the evals-side docs ship in the evals PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughChangesObservability controls and reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant register
participant globalThis
participant HealthGET
participant HealthClient
register->>globalThis: Set __polymorphTracingState
HealthClient->>HealthGET: Request check=phoenix or check=all
HealthGET->>globalThis: Read tracing state
globalThis-->>HealthGET: Return state or unknown
HealthGET-->>HealthClient: Return tracing metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e5b46d191
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+389
to
+390
| // traces most worth keeping. Runs after persistence so spans include | ||
| // DB write latency. The 5s timeout (default) is small relative to the |
There was a problem hiding this comment.
For successful chats, withOtelRootSpan finishes at line 355 before onFinish starts, and persistStreamResults runs later at lines 361–378 outside that span. flushTraces() only exports spans that were already recorded; it cannot add the intervening DB-write duration, so Phoenix traces still exclude persistence latency. This new comment—and the matching statement in STREAMING-OPERATIONS.md—will mislead operators diagnosing slow completions; either instrument persistence within an active span or remove the claim.
Useful? React with 👍 / 👎.
The wording introduced earlier in this PR claimed "Root-span and tool-event attributes are unaffected." Half of that is wrong and the other half reads as reassurance when it is actually the caveat. The root span really is unaffected — because `withOtelRootSpan` in create-chat-stream-response.ts builds it outside the AI SDK, so telemetryRecordingOptions() structurally cannot reach it. It keeps emitting session id, user id, and request metadata no matter how the flags are set. That is a limit to document, not a comfort. Masking removes message content, not who sent it. The tool-event half is unverified and was dropped rather than replaced with another specific claim. Also documents that only the exact string "true" masks, so the control fails toward recording rather than toward hiding — the direction that matters for a privacy switch. This PR exists because these variables were documented as doing something they did not do. Shipping a fresh inaccurate claim about them would repeat exactly that mistake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/utils/telemetry.test.ts (1)
154-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore stubbed environment values in test cleanup.
vi.unstubAllEnvs()runs only after the first assertion succeeds. If an assertion fails, the stubs can leak into later tests and cause cascading failures; move cleanup toafterEachor afinallyblock.Proposed fix
describe('telemetryRecordingOptions', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + it('telemetryRecordingOptions honors OPENINFERENCE_HIDE_INPUTS/OUTPUTS', () => { vi.stubEnv('OPENINFERENCE_HIDE_INPUTS', 'true') vi.stubEnv('OPENINFERENCE_HIDE_OUTPUTS', 'false') @@ - vi.unstubAllEnvs() expect(telemetryRecordingOptions()).toEqual({ recordInputs: true, recordOutputs: true🤖 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 `@lib/utils/telemetry.test.ts` around lines 154 - 168, Move the vi.unstubAllEnvs cleanup out of the telemetryRecordingOptions test body and into an afterEach hook for the describe block, ensuring environment stubs are restored even when an assertion fails. Keep the existing assertions and expected telemetryRecordingOptions behavior unchanged.
🤖 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 `@app/api/health/route.test.ts`:
- Around line 25-74: Make the GET /api/health tests deterministic by disabling
tracing in beforeEach, or explicitly mocking fetch and the Phoenix collector
endpoint so no test performs a real /healthz request when ENABLE_TRACING and
PHOENIX_COLLECTOR_ENDPOINT are inherited from the environment. Preserve the
existing assertions for global tracing state and response contents.
---
Nitpick comments:
In `@lib/utils/telemetry.test.ts`:
- Around line 154-168: Move the vi.unstubAllEnvs cleanup out of the
telemetryRecordingOptions test body and into an afterEach hook for the describe
block, ensuring environment stubs are restored even when an assertion fails.
Keep the existing assertions and expected telemetryRecordingOptions behavior
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e91ea4b3-9655-476c-a9ec-935ea4be4815
📒 Files selected for processing (22)
.env.local.exampleapp/api/health/route.test.tsapp/api/health/route.tsdocs/architecture/STREAMING-OPERATIONS.mddocs/getting-started/ENVIRONMENT-OPERATIONS.mddocs/operations/PHOENIX-OPERATIONS.mddocs/reference/API-AUXILIARY-ENDPOINTS.mdinstrumentation.test.tsinstrumentation.tslib/agents/__tests__/researcher.test.tslib/agents/__tests__/title-generator.test.tslib/agents/chat/__tests__/registry.test.tslib/agents/chat/factory.tslib/agents/generate-related-questions.tslib/agents/generate-trending-suggestions.tslib/agents/title-generator.tslib/config/env.tslib/streaming/__tests__/create-chat-stream-response.test.tslib/streaming/create-chat-stream-response.tslib/tools/generate-image/server.tslib/utils/telemetry.test.tslib/utils/telemetry.ts
💤 Files with no reviewable changes (1)
- lib/config/env.ts
Comment on lines
+25
to
+74
| describe('GET /api/health', () => { | ||
| it('includes tracing in the body for check=phoenix, reflecting the current global state', async () => { | ||
| mockExecute.mockResolvedValue(undefined) | ||
| globalThis.__polymorphTracingState = 'disabled-https' | ||
|
|
||
| const response = await GET(makeRequest('?check=phoenix')) | ||
| const body = await response.json() | ||
|
|
||
| expect(body.tracing).toBe('disabled-https') | ||
| }) | ||
|
|
||
| it('includes tracing in the body for check=all, reflecting the current global state', async () => { | ||
| mockExecute.mockResolvedValue(undefined) | ||
| globalThis.__polymorphTracingState = 'enabled' | ||
|
|
||
| const response = await GET(makeRequest('?check=all')) | ||
| const body = await response.json() | ||
|
|
||
| expect(body.tracing).toBe('enabled') | ||
| }) | ||
|
|
||
| it('omits tracing from the body when check is unset', async () => { | ||
| mockExecute.mockResolvedValue(undefined) | ||
| globalThis.__polymorphTracingState = 'enabled' | ||
|
|
||
| const response = await GET(makeRequest()) | ||
| const body = await response.json() | ||
|
|
||
| expect(body).not.toHaveProperty('tracing') | ||
| }) | ||
|
|
||
| it('omits tracing from the body for an unrecognized check value', async () => { | ||
| mockExecute.mockResolvedValue(undefined) | ||
| globalThis.__polymorphTracingState = 'enabled' | ||
|
|
||
| const response = await GET(makeRequest('?check=db')) | ||
| const body = await response.json() | ||
|
|
||
| expect(body).not.toHaveProperty('tracing') | ||
| }) | ||
|
|
||
| it('falls back to unknown when the global tracing state was never set', async () => { | ||
| mockExecute.mockResolvedValue(undefined) | ||
|
|
||
| const response = await GET(makeRequest('?check=phoenix')) | ||
| const body = await response.json() | ||
|
|
||
| expect(body.tracing).toBe('unknown') | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make Phoenix-check tests independent of inherited environment.
If CI provides both ENABLE_TRACING=true and PHOENIX_COLLECTOR_ENDPOINT, these tests perform a real /healthz fetch. Stub tracing off in beforeEach, or mock fetch and the endpoint explicitly.
🤖 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 `@app/api/health/route.test.ts` around lines 25 - 74, Make the GET /api/health
tests deterministic by disabling tracing in beforeEach, or explicitly mocking
fetch and the Phoenix collector endpoint so no test performs a real /healthz
request when ENABLE_TRACING and PHOENIX_COLLECTOR_ENDPOINT are inherited from
the environment. Preserve the existing assertions for global tracing state and
response contents.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62088cef2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `[onFinish] Failed to persist stream results for chat ${chatId}:`, | ||
| error | ||
| ) | ||
| if (!isAborted && responseMessage) { |
There was a problem hiding this comment.
When a new chat is aborted while generateChatTitle is still running, this branch skips persistStreamResults, which is the only path that awaits titlePromise, and immediately calls flushTraces(). The flush can therefore finish before the title-generation span ends; once the serverless invocation terminates, that span is still lost. Await the already error-handled titlePromise before flushing aborted streams so the new abort coverage includes this parallel LLM call.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Four independent gaps in Phoenix tracing, all of which fail silently — which is why none of them had been noticed.
What changed
1. Aborted chats lost every span.
onFinishreturned early on abort, beforeawait flushTraces(). On Vercel the function terminates as soon as the response settles, so an aborted chat's spans died unexported. Aborted chats are exactly the traces worth having — they are where users bail out. The flush now runs in afinally; persistence still skips aborted and empty responses. Regression tests cover the abort path, the missing-responseMessagepath, and the case where persistence throws.2.
OPENINFERENCE_HIDE_*masked nothing. The vars were documented as hiding prompt and completion content, but nothing in this setup reads them —openinference-vercel's span processor doesn't consume them here. An operator could set them, see no error, and keep shipping every user prompt to Phoenix. The AI SDK does honour per-callrecordInputs/recordOutputs, so atelemetryRecordingOptions()helper is now spread into everyexperimental_telemetryblock and is the single enforcement point. Five vars that will never be honoured (including the non-HIDE_-prefixedOPENINFERENCE_BASE64_IMAGE_MAX_LENGTH) are deleted from both lists inlib/config/env.tsand from the docs — leaving them documented is the exact failure this fixes.3. Two LLM calls emitted no spans at all. The app makes five LLM calls; three were instrumented. Image generation and trending-suggestions were invisible in Phoenix — including the trending-suggestions call that runs on a Vercel cron, where there is no UI for anyone to notice a failure. All five call sites are now instrumented and uniform.
4. A blind deploy looked healthy.
instrumentation.tsdeliberately disables tracing when the collector endpoint is plain HTTP in production. It did so silently, and/api/healthreportedphoenix: 'ok'because the collector was reachable — while this process had registered no exporter./api/healthnow also returnstracing, so the blind-deploy signaturephoenix: 'ok'+tracing: 'disabled-https'is visible.Verification
bun lint,bun typecheck, and the full root suite all clean on this branch standalone: 188 test files, 1572 tests, 0 failures.This change makes
OPENINFERENCE_HIDE_INPUTS/OPENINFERENCE_HIDE_OUTPUTSwork for the first time. If either is already set totruein Vercel production — plausibly set at some point in the belief that it was already masking — merging this will silently blank prompt and completion content on every AI SDK span in Phoenix. Check the current values first. That is a decision to make deliberately, not to discover in the trace viewer.Setting any of the five deleted vars after this merge is inert, exactly as it was before.
Runtime notes
flushTraces()cannot throw or hang past 5s: it early-returns when tracing is off, wraps its body in a try/catch that only warns, and racesforceFlush()against a timer that resolves rather than rejects. Moving it intofinallycannot convert a persistence failure into an unhandled rejection.globalThis.__polymorphTracingStateis written once duringregister()at process init and read-only thereafter, so there is no concurrent-write hazard. Each serverless instance sets its own — which is the correct semantics, since the field means "did this process register an exporter." Consequence worth knowing:/api/health?check=phoenixreports whichever instance answered, so oneenabledresponse does not prove every instance is exporting. Sample a few times when diagnosing.Known follow-ups (not blocking)
enabledandinit-failedtracing states have no test coverage; reaching them requires mocking the full OTel stack. The two reachable states (disabled-off,disabled-https) and the whole health-route field are covered, and thedisabled-offtest is ordering-sensitive — it fails if the default assignment moves after theENABLE_TRACINGcheck.TracingStateis declared identically ininstrumentation.tsandapp/api/health/route.ts. TypeScript errors if the two drift, so it is self-policing, but a shared export would be tidier.Review notes
Commits were cherry-picked from the working branch
evals/pipeline-restore-and-quality, which also carries an unrelated eval-harness change set. That half ships separately in #262. The two surfaces share no files and no data dependencies.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation