Open
Conversation
TS's noUnusedLocals does not exempt _-prefixed plain const/function declarations, only destructured locals (TS 4.4+). Re-add explicit void markers for stateByCtx, safeCall, and the two histogram consts until later tasks wire them in.
Also fixes ESLint errors in otel.ts (import order, type param naming, array-type, unnecessary nullish coalescing and optional chain).
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an OpenTelemetry middleware (otelMiddleware), documentation, tests, e2e capture, and package exports. The middleware emits root chat spans with per-iteration and tool child spans, records duration and token histograms, supports optional content capture with redaction/truncation, and exposes extension hooks; OTEL is an optional peer dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Chat Client
participant Middleware as otelMiddleware
participant Tracer as OpenTelemetry Tracer
participant Meter as OpenTelemetry Meter
Client->>Middleware: onStart(chat)
Middleware->>Tracer: startSpan("chat")
Client->>Middleware: onConfig(beforeModel)
Middleware->>Tracer: startSpan("iteration")
Client->>Middleware: onBeforeToolCall(tool)
Middleware->>Tracer: startSpan("tool")
Client->>Middleware: onAfterToolCall(toolResult)
Middleware->>Tracer: endSpan("tool")
Client->>Middleware: onChunk / streaming
Middleware->>Tracer: add events (messages/choices)
Client->>Middleware: onUsage(usage)
Middleware->>Meter: record token histogram(s)
Client->>Middleware: onFinish()
Middleware->>Tracer: endSpan("iteration")
Middleware->>Tracer: endSpan("chat")
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
🚀 Changeset Version Preview11 package(s) bumped directly, 22 bumped as dependents. 🟥 Major bumps
🟨 Minor bumps
🟩 Patch bumps
|
|
View your CI Pipeline Execution ↗ for commit 5359a60
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit d6134d2 ☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
![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: 8
🧹 Nitpick comments (6)
packages/typescript/ai/src/middlewares/otel.ts (5)
423-477:info.error as Errorcasts may drop metadata for non-Error throwables.
recordExceptionin@opentelemetry/apiacceptsException = string | ExceptionWithCode | ExceptionWithMessage | ExceptionWithName | Error. Casting an arbitraryunknowntoErrorworks at the TS level but loses the richer shape if the thrown value is, e.g., a plain{ code, message }object. Just drop the cast and let OTel handle it:- state.currentIterationSpan.recordException(info.error as Error) + state.currentIterationSpan.recordException(info.error as Exception) ... - state.rootSpan.recordException(info.error as Error) + state.rootSpan.recordException(info.error as Exception)(import
type { Exception }from@opentelemetry/api). Same applies at lines 388 and 452. This is cosmetic — no runtime bug — but avoids silently stringifying{}errors in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 423 - 477, The onError handler is casting info.error to Error before calling recordException which discards richer non-Error throwables; import type { Exception } from '@opentelemetry/api' and change the recordException calls in onError (and the similar places referenced around the other recordException usages at the locations corresponding to lines near 388 and 452) to pass info.error as the Exception type (i.e., don’t cast to Error) for state.currentIterationSpan.recordException, each span.recordException in the state.toolSpans loop, and state.rootSpan.recordException so OTel can preserve non-Error shapes.
265-288:gen_ai.choiceis never emitted for tool-only iterations.If an iteration ends with
finishReason: 'tool_calls'and noTEXT_MESSAGE_CONTENTchunks (the model only produced tool calls),assistantTextBufferstays empty and nogen_ai.choiceevent is recorded, but the GenAI semconv still expects a choice event carrying the tool-call list. Not a blocker for the first release — text-only runs work correctly — but worth tracking as a follow-up so observability tools don't see "missing" choices on tool-calling iterations. Consider also bufferingTOOL_CALL_*chunks and emitting a structuredgen_ai.choicewithtool_callsattribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 265 - 288, When an iteration ends with finishReason 'tool_calls' but no TEXT_MESSAGE_CONTENT, no gen_ai.choice is emitted; update the logic around state.currentIterationSpan handling (where chunk.type and state.assistantTextBuffer are used) to also collect TOOL_CALL_* chunks into a temporary buffer (e.g., state.toolCallBuffer) during streaming, and when chunk.type === 'RUN_FINISHED' emit a gen_ai.choice event even if assistantTextBuffer is empty by adding a structured payload with a tool_calls attribute (redacting via the existing safeCall('otel.redact', () => redact(...)) pattern or falling back) on span.addEvent; clear both assistantTextBuffer and the new toolCallBuffer after emitting.
112-135: LGTM — options destructuring and histogram wiring.Optional
meteris handled cleanly viameter?.createHistogram(...). Units's'for duration and'{token}'for token count both align with OTel GenAI semantic convention._serviceNameis destructured but unused; consider removing it from the options interface if it's not consumed elsewhere, or wire it to aservice.nameresource attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 112 - 135, The destructured _serviceName in otelMiddleware (from OtelMiddlewareOptions) is never used; either remove it from the OtelMiddlewareOptions type and the destructuring in otelMiddleware, or wire it into OTel resource/span attributes by applying it as the service.name (e.g., pass it into created spans or the tracer/resource so spans include 'service.name')—update the OtelMiddlewareOptions definition and the otelMiddleware function accordingly to keep the shape consistent with the change.
139-172: Root span isn't captured with the iteration/tool spans' active context.
onStartcreatesrootSpanviatracer.startSpan(name, spanOptions)without making it the active span. That's fine becauseonConfiglater doesotelContext.with(otelTrace.setSpan(otelContext.active(), state.rootSpan), …)to create the iteration span as a child. However, if any external code (or another middleware) runs betweenonStartandonConfigand creates its own spans via the active context API, those spans will not be parented torootSpan. If you want the root span to genuinely be the active parent for the whole run, consider usingtracer.startActiveSpanpattern or storing the root context onstatesoonConfig/onBeforeToolCallcan restore it even if the OTel active context has moved on. Not blocking, but worth a thought for deep integrations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 139 - 172, The root span is started with tracer.startSpan inside onStart which doesn't make it the active OTel context, so subsequent spans started elsewhere may not be parented to it; change onStart to either use tracer.startActiveSpan (so the root span becomes active immediately) or save the root span's context on the state object (e.g., store otelTrace.setSpan(otelContext.active(), rootSpan) as rootContext in stateByCtx) and then in onConfig, onBeforeToolCall and any other span-creating places restore that saved context with otelContext.with(rootContext, ...) before starting child spans; update references to stateByCtx, state.rootSpan, onConfig and onBeforeToolCall accordingly so child spans are always created under the intended root.
302-328:onUsagesilently loses data when called outside an iteration.If the adapter emits
onUsagebeforeonConfig(beforeModel)has opened an iteration span (or afterRUN_FINISHEDclosed it), the!state.currentIterationSpanguard returns early — no histogram record, no attribute set. The tokens simply vanish. Acceptable as a defensive no-op, but since token usage on the root span is only populated fromonFinish(info.usage), an adapter that never passesusagein the finish payload and only emitsonUsageafter iteration close will result in zero token attributes anywhere. Consider at minimum still recording the token histogram even when the iteration span is gone — it doesn't need a span target.- if (!state || !state.currentIterationSpan) return - - state.currentIterationSpan.setAttributes({ - 'gen_ai.usage.input_tokens': usage.promptTokens, - 'gen_ai.usage.output_tokens': usage.completionTokens, - }) + if (!state) return + state.currentIterationSpan?.setAttributes({ + 'gen_ai.usage.input_tokens': usage.promptTokens, + 'gen_ai.usage.output_tokens': usage.completionTokens, + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 302 - 328, The onUsage handler currently returns early when state.currentIterationSpan is missing, dropping token data; change it so tokenHistogram.record calls always run (outside the currentIterationSpan guard) so metrics are recorded even if no iteration span exists, and if no currentIterationSpan is present set the token attributes on the root span (e.g., state.rootSpan or state.span) instead of skipping them; keep the same metricAttrs and attribute keys, and leave setting attributes on currentIterationSpan unchanged when it exists.packages/typescript/ai/tests/middlewares/fake-otel.ts (1)
168-196: FakeMeteris missing newer methods that real Meter requires.
createGauge,addBatchObservableCallback,removeBatchObservableCallbackare present, but the castas Meteris doing the heavy lifting. That's fine for this test, but note thatcreateHistogramhere ignores the second parameter (options like{ description, unit }), which the productionotelMiddlewaredoes pass — so tests can't catch regressions if someone misconfigures unit/description. Optional: capture options too for future-proofing.- createHistogram(name: string): Histogram { + createHistogram(name: string, _options?: unknown): Histogram {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/tests/middlewares/fake-otel.ts` around lines 168 - 196, The fake Meter implementation (meter.createHistogram) ignores the optional second parameter (options like {description, unit}) used by production otelMiddleware, so tests can't detect regressions; update the meter.createHistogram signature to accept (name: string, options?: HistogramOptions) and have the returned record function push the options into records (e.g., records.push({ name, value, attributes, options })) so tests can assert unit/description, and keep references to meter, createHistogram, record, records and otelMiddleware to locate and validate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/advanced/otel.md`:
- Around line 52-58: The docs claim iteration spans are kind: CLIENT but
otelMiddleware currently calls tracer.startSpan(name, spanOptions) without
setting options.kind so spans default to INTERNAL; to fix, either (A) change
otelMiddleware to pass { kind: SpanKind.CLIENT } when starting iteration spans
(import SpanKind from '@opentelemetry/api' and add the kind to the spanOptions
used for iteration spans in otelMiddleware/tracer.startSpan), or (B) update the
docs to show iteration spans as INTERNAL to match current behavior; also add a
language hint (e.g., text) to the fenced code block in docs/advanced/otel.md to
satisfy MD040.
- Around line 23-25: Update the fenced code block that contains the install
command "pnpm add `@opentelemetry/api`" to include a language hint (use bash or
sh) so the snippet becomes a bash code block; locate the fenced block in
docs/advanced/otel.md and change the opening triple backticks to include "bash"
for consistency with other docs and to satisfy markdownlint MD040.
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 102-110: safeCall currently swallows all exceptions (it voids err
and label) which contradicts the docs; update safeCall to catch the error and
emit a dev-visible warning or pass the error to a configured debug callback
instead of silencing it. Specifically, in function safeCall<T>(label: string,
fn: () => T): T | undefined, log the caught error along with the label via the
existing logger or invoke a debug hook (gated by an environment flag like
OTEL_DEBUG or an options.debug callback exposed from the otel middleware) so
attributeEnricher/user callbacks failures are observable while preserving the
try/catch behavior described in docs. Ensure the change does not throw and still
returns undefined on error.
- Around line 330-421: The review notes that tool spans stored in
state.toolSpans (keyed by toolCallId) can leak if onAfterToolCall is never
invoked; add a defensive sweep in onFinish that mirrors the cleanup in
onError/onAbort to iterate state.toolSpans, set outcome/exception/status as
appropriate, end each Span, and clear the map before ending the iteration span
(use the same logic that touches state.currentIterationSpan and state.toolSpans
as in onError/onAbort); update the onFinish handler to perform this sweep so any
leftover spans are closed when the iteration finishes.
- Around line 221-231: Import SpanKind from "@opentelemetry/api" and set
span.kind appropriately: update the baseOptions / spanOptions used when creating
iteration spans (where tracer.startSpan(name, spanOptions) and iterSpan are
created) to include kind: SpanKind.CLIENT, and update the root span creation
(the code that sets state.rootSpan earlier, referenced around the root span
initialization) to include kind: SpanKind.INTERNAL so the runtime spans match
the docs and the explicit parent assignment to state.rootSpan remains correct.
- Around line 260-300: The onChunk handler in otel.ts is comparing chunk.type to
string literals ('TEXT_MESSAGE_CONTENT', 'RUN_FINISHED'), which is brittle;
update the comparisons to use the EventType enum imported from "@ag-ui/core"
(e.g., compare chunk.type === EventType.TEXT_MESSAGE_CONTENT and chunk.type ===
EventType.RUN_FINISHED), add the EventType import at the top of the file, and
keep existing logic in onChunk (stateByCtx lookup, assistantTextBuffer capture,
span attribute/event setting, onSpanEnd call and span.end()) so chunk type
checks use the enum values for correctness and type safety.
In `@packages/typescript/ai/tests/middlewares/fake-otel.ts`:
- Around line 1-14: Reorder and clean imports and types to satisfy ESLint: move
the value import SpanStatusCode before the type-only imports and alphabetically
sort the type imports so AttributeValue is in order; replace every array
shorthand (e.g., Foo[]) with the generic form Array<Foo> throughout this file to
address `@typescript-eslint/array-type`; and add a top-level import type for
ChatMiddlewareContext (replace inline import('...').ChatMiddlewareContext usages
with the top-level import type) to satisfy
`@typescript-eslint/consistent-type-imports`; after making these changes run the
linter to confirm no remaining sort-imports/array-type/consistent-type-imports
errors.
In `@packages/typescript/ai/tests/middlewares/otel.test.ts`:
- Around line 669-685: The test currently verifies that
otelMiddleware.onStart/onFinish don't throw when attributeEnricher throws, but
the error is swallowed silently by safeCall; update the implementation so thrown
callback errors are logged and the test to assert that logging occurs: modify
safeCall (the utility used by otelMiddleware) to catch exceptions and call
console.warn (including the error and a label/context), then update the test for
attributeEnricher in otel.test.ts to spyOn console.warn (vi.spyOn(console,
'warn')) and expect it to have been called when mw.onStart/onFinish are invoked;
reference attributeEnricher, otelMiddleware, safeCall, and onStart/onFinish when
making these changes.
---
Nitpick comments:
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 423-477: The onError handler is casting info.error to Error before
calling recordException which discards richer non-Error throwables; import type
{ Exception } from '@opentelemetry/api' and change the recordException calls in
onError (and the similar places referenced around the other recordException
usages at the locations corresponding to lines near 388 and 452) to pass
info.error as the Exception type (i.e., don’t cast to Error) for
state.currentIterationSpan.recordException, each span.recordException in the
state.toolSpans loop, and state.rootSpan.recordException so OTel can preserve
non-Error shapes.
- Around line 265-288: When an iteration ends with finishReason 'tool_calls' but
no TEXT_MESSAGE_CONTENT, no gen_ai.choice is emitted; update the logic around
state.currentIterationSpan handling (where chunk.type and
state.assistantTextBuffer are used) to also collect TOOL_CALL_* chunks into a
temporary buffer (e.g., state.toolCallBuffer) during streaming, and when
chunk.type === 'RUN_FINISHED' emit a gen_ai.choice event even if
assistantTextBuffer is empty by adding a structured payload with a tool_calls
attribute (redacting via the existing safeCall('otel.redact', () => redact(...))
pattern or falling back) on span.addEvent; clear both assistantTextBuffer and
the new toolCallBuffer after emitting.
- Around line 112-135: The destructured _serviceName in otelMiddleware (from
OtelMiddlewareOptions) is never used; either remove it from the
OtelMiddlewareOptions type and the destructuring in otelMiddleware, or wire it
into OTel resource/span attributes by applying it as the service.name (e.g.,
pass it into created spans or the tracer/resource so spans include
'service.name')—update the OtelMiddlewareOptions definition and the
otelMiddleware function accordingly to keep the shape consistent with the
change.
- Around line 139-172: The root span is started with tracer.startSpan inside
onStart which doesn't make it the active OTel context, so subsequent spans
started elsewhere may not be parented to it; change onStart to either use
tracer.startActiveSpan (so the root span becomes active immediately) or save the
root span's context on the state object (e.g., store
otelTrace.setSpan(otelContext.active(), rootSpan) as rootContext in stateByCtx)
and then in onConfig, onBeforeToolCall and any other span-creating places
restore that saved context with otelContext.with(rootContext, ...) before
starting child spans; update references to stateByCtx, state.rootSpan, onConfig
and onBeforeToolCall accordingly so child spans are always created under the
intended root.
- Around line 302-328: The onUsage handler currently returns early when
state.currentIterationSpan is missing, dropping token data; change it so
tokenHistogram.record calls always run (outside the currentIterationSpan guard)
so metrics are recorded even if no iteration span exists, and if no
currentIterationSpan is present set the token attributes on the root span (e.g.,
state.rootSpan or state.span) instead of skipping them; keep the same
metricAttrs and attribute keys, and leave setting attributes on
currentIterationSpan unchanged when it exists.
In `@packages/typescript/ai/tests/middlewares/fake-otel.ts`:
- Around line 168-196: The fake Meter implementation (meter.createHistogram)
ignores the optional second parameter (options like {description, unit}) used by
production otelMiddleware, so tests can't detect regressions; update the
meter.createHistogram signature to accept (name: string, options?:
HistogramOptions) and have the returned record function push the options into
records (e.g., records.push({ name, value, attributes, options })) so tests can
assert unit/description, and keep references to meter, createHistogram, record,
records and otelMiddleware to locate and validate the change.
🪄 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
Run ID: df65c091-c486-4d70-873f-6667fc942989
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/otel-middleware.mddocs/advanced/otel.mddocs/config.jsonpackages/typescript/ai/package.jsonpackages/typescript/ai/src/middlewares/index.tspackages/typescript/ai/src/middlewares/otel.tspackages/typescript/ai/tests/middlewares/fake-otel.tspackages/typescript/ai/tests/middlewares/otel.test.ts
Comment on lines
+260
to
+300
| onChunk(ctx, chunk) { | ||
| safeCall('otel.onChunk', () => { | ||
| const state = stateByCtx.get(ctx) | ||
| if (!state) return | ||
|
|
||
| if (captureContent && chunk.type === 'TEXT_MESSAGE_CONTENT') { | ||
| state.assistantTextBuffer += chunk.delta | ||
| } | ||
|
|
||
| if (chunk.type !== 'RUN_FINISHED') return | ||
| if (!state.currentIterationSpan) return | ||
| const span = state.currentIterationSpan | ||
|
|
||
| if (chunk.finishReason) { | ||
| span.setAttribute('gen_ai.response.finish_reasons', [ | ||
| chunk.finishReason, | ||
| ]) | ||
| } | ||
| if (chunk.model) span.setAttribute('gen_ai.response.model', chunk.model) | ||
|
|
||
| if (captureContent && state.assistantTextBuffer.length > 0) { | ||
| span.addEvent('gen_ai.choice', { | ||
| content: | ||
| safeCall('otel.redact', () => | ||
| redact(state.assistantTextBuffer), | ||
| ) ?? state.assistantTextBuffer, | ||
| }) | ||
| state.assistantTextBuffer = '' | ||
| } | ||
|
|
||
| safeCall('otel.onSpanEnd', () => | ||
| onSpanEnd?.( | ||
| { kind: 'iteration', ctx, iteration: state.iterationCount - 1 }, | ||
| span, | ||
| ), | ||
| ) | ||
| span.end() | ||
| state.currentIterationSpan = null | ||
| }) | ||
| return undefined | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm EventType's member values for TEXT_MESSAGE_CONTENT and RUN_FINISHED.
fd -t f 'types.ts' packages/typescript/ai/src | head -20
rg -nP -C2 '\bEventType\b' packages/typescript/ai/src/types.ts | head -80
rg -nP -C1 "TEXT_MESSAGE_CONTENT|RUN_FINISHED" packages/typescript/ai/src/types.tsRepository: TanStack/ai
Length of output: 1154
🏁 Script executed:
# Check the otel.ts file to see current imports and usage
cat -n packages/typescript/ai/src/middlewares/otel.ts | head -50Repository: TanStack/ai
Length of output: 1640
🏁 Script executed:
# Search for EventType enum definition in `@ag-ui/core` or its values in the codebase
rg -n "TEXT_MESSAGE_CONTENT|RUN_FINISHED" packages/typescript/ai/src --type ts -B2 -A2 | head -100Repository: TanStack/ai
Length of output: 8299
🏁 Script executed:
# Check what's currently imported in otel.ts
rg -n "^import" packages/typescript/ai/src/middlewares/otel.tsRepository: TanStack/ai
Length of output: 98
🏁 Script executed:
# Search for EventType definition and its member values
rg -n "enum EventType|TEXT_MESSAGE_CONTENT|RUN_FINISHED" packages/typescript/ai/src/types.ts -B1 -A1Repository: TanStack/ai
Length of output: 37
🏁 Script executed:
# Count files using string literals vs EventType enum for these specific checks
echo "=== Files using string literal 'TEXT_MESSAGE_CONTENT' ===" && \
rg -l "chunk\.type === ['\"]TEXT_MESSAGE_CONTENT['\"]|chunk\.type !== ['\"]TEXT_MESSAGE_CONTENT['\"]" packages/typescript/ai/src --type ts
echo "=== Files using EventType.TEXT_MESSAGE_CONTENT ===" && \
rg -l "EventType\.TEXT_MESSAGE_CONTENT" packages/typescript/ai/src --type ts
echo "=== Files using string literal 'RUN_FINISHED' ===" && \
rg -l "chunk\.type [!=]== ['\"]RUN_FINISHED['\"]" packages/typescript/ai/src --type ts
echo "=== Files using EventType.RUN_FINISHED ===" && \
rg -l "EventType\.RUN_FINISHED" packages/typescript/ai/src --type tsRepository: TanStack/ai
Length of output: 692
🏁 Script executed:
# Look for test files that might define or use EventType
fd -t f 'test|spec' packages/typescript/ai/src --type ts | xargs rg -l "EventType" 2>/dev/null | head -10Repository: TanStack/ai
Length of output: 754
🏁 Script executed:
# Search for where EventType enum members are defined or documented
rg -n "EventType\." packages/typescript/ai/src --type ts -A1 | head -50Repository: TanStack/ai
Length of output: 973
🏁 Script executed:
# Check the `@ag-ui/core` import to see if there's any indication of enum values
cat packages/typescript/ai/src/types.ts | grep -A20 "from '@ag-ui/core'"Repository: TanStack/ai
Length of output: 1305
🏁 Script executed:
# Check the test file to see how they use EventType
head -200 packages/typescript/ai/tests/middlewares/otel.test.tsRepository: TanStack/ai
Length of output: 6240
🏁 Script executed:
# Also look for any place where chunk objects are created to understand the type
rg -n "type: EventType|type: 'TEXT_MESSAGE_CONTENT'|type: 'RUN_FINISHED'" packages/typescript/ai/tests/middlewares/otel.test.ts -B1 -A1Repository: TanStack/ai
Length of output: 1100
Use EventType enum instead of string-literal comparisons for chunk types.
The test suite constructs chunks with type: EventType.RUN_FINISHED and type: EventType.TEXT_MESSAGE_CONTENT, but the middleware compares against string literals. If the enum values ever change in @ag-ui/core, these comparisons will silently fail—content won't be captured and spans won't close properly. Import and use the enum members directly for consistency and type safety.
Suggested changes
+import { EventType } from '../types'
...
- if (captureContent && chunk.type === 'TEXT_MESSAGE_CONTENT') {
+ if (captureContent && chunk.type === EventType.TEXT_MESSAGE_CONTENT) {
state.assistantTextBuffer += chunk.delta
}
- if (chunk.type !== 'RUN_FINISHED') return
+ if (chunk.type !== EventType.RUN_FINISHED) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 260 - 300, The
onChunk handler in otel.ts is comparing chunk.type to string literals
('TEXT_MESSAGE_CONTENT', 'RUN_FINISHED'), which is brittle; update the
comparisons to use the EventType enum imported from "@ag-ui/core" (e.g., compare
chunk.type === EventType.TEXT_MESSAGE_CONTENT and chunk.type ===
EventType.RUN_FINISHED), add the EventType import at the top of the file, and
keep existing logic in onChunk (stateByCtx lookup, assistantTextBuffer capture,
span attribute/event setting, onSpanEnd call and span.end()) so chunk type
checks use the enum values for correctness and type safety.
… docs - Set SpanKind.INTERNAL on root and tool spans, SpanKind.CLIENT on iteration spans so traces match the OTel GenAI convention and render correctly in backends that filter by kind. - Fire onSpanEnd for tool spans in onError and onAbort drain loops (was previously only called from onAfterToolCall). - Preserve toolName on tool-span state so error/abort onSpanEnd callbacks receive complete OtelSpanInfo. - Remove unused serviceName option from OtelMiddlewareOptions (never wired). - Correct duration-histogram scope in docs: chat() operation, not per-iteration.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/typescript/ai/src/middlewares/otel.ts (2)
555-565:⚠️ Potential issue | 🟡 MinorSweep dangling tool spans on normal finish before closing the parent.
If a tool span is opened but
onAfterToolCallnever runs,onFinishdeletes the request state without ending that span. Close any remaining tool spans before ending a dangling iteration span.Suggested change
- // Close a dangling iteration span if RUN_FINISHED never arrived (defensive). + // Close any dangling tool spans before their parent iteration span. + for (const [id, entry] of state.toolSpans) { + const { span, toolName } = entry + span.setAttribute('tanstack.ai.tool.outcome', 'unknown') + safeCall('otel.onSpanEnd', () => + onSpanEnd?.( + { + kind: 'tool', + ctx, + toolCallId: id, + toolName, + iteration: state.iterationCount - 1, + } as OtelSpanInfo<'tool'>, + span, + ), + ) + span.end() + state.toolSpans.delete(id) + } + + // Close a dangling iteration span if RUN_FINISHED never arrived (defensive). if (state.currentIterationSpan) { safeCall('otel.onSpanEnd', () => onSpanEnd?.(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 555 - 565, Before ending a dangling iteration span in the block that checks state.currentIterationSpan, first iterate and close any remaining tool spans in state.currentToolSpans (the spans that would normally be closed by onAfterToolCall) so they aren't leaked; for each tool span call safeCall('otel.onSpanEnd', () => onSpanEnd?.({ kind: 'tool', ctx, /* include any identifier available */ }, toolSpan)) and then toolSpan.end(), clearing state.currentToolSpans, and only after that invoke onSpanEnd for the iteration ({ kind: 'iteration', ctx, iteration: state.iterationCount - 1 }), end state.currentIterationSpan and set it to null to ensure tool spans are ended before the parent iteration span.
102-109:⚠️ Potential issue | 🟡 MinorSurface callback failures instead of making them invisible.
safeCallcurrently discards both the label and the error, so broken extension callbacks become impossible to diagnose while the chat continues.Suggested change
function safeCall<T>(label: string, fn: () => T): T | undefined { try { return fn() } catch (err) { - void err - void label + // Keep middleware non-fatal, but make extension failures observable. + // eslint-disable-next-line no-console + console.warn(`[otelMiddleware] ${label} failed`, err) return undefined } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 102 - 109, safeCall currently swallows errors and the label, making extension callback failures invisible; update the safeCall(label, fn) implementation to surface failures by logging the label and the caught error (including err.stack when available) before returning undefined (or rethrowing if you prefer); reference the safeCall function and its parameters label and err so you can replace the void err/void label no-ops with a meaningful call to your logger (e.g., console.error or the project's logger) that emits both the label and error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 243-255: When captureContent is true, ensure redact and
serializeContent never cause raw content to be emitted: wrap calls to
redact(...) and serializeContent(...) in try/catch (or centralize via safeCall)
and on any error replace content with a non-sensitive sentinel like
"<redaction_failed>" or "<serialization_failed>" before calling
iterSpan.addEvent (use messageEventName, gen_ai.system.message, or the tool-span
events), and ensure iterSpan.addEvent is always executed so spans are finalized;
apply the same hardening to the other similar blocks referenced (around the
logic in captureContent at the locations matching 283-289 and 405-413) and
reference functions used: captureContent, redact, serializeContent, safeCall,
iterSpan.addEvent, and messageEventName.
- Around line 502-548: onAbort currently ignores the info/duration and never
emits the gen_ai.client.operation.duration metric; update the onAbort handler
(change parameter from _info to info) to extract info.duration and record that
duration to the same gen_ai.client.operation.duration metric used by
successful/error paths (use the project's existing metrics helper used elsewhere
in this file) before closing/ending spans (i.e., before calling
closeCancelled(state.rootSpan) and ending spans like state.currentIterationSpan,
entries in state.toolSpans, and state.rootSpan); keep the existing cancelled
tag/status on spans and include a status or label (e.g., "cancelled") when
recording the metric so cancelled runs are visible in latency metrics.
- Around line 276-300: The onChunk handler currently closes the iteration span
(state.currentIterationSpan = null and span.end()) whenever a chunk has a
finishReason, which prematurely closes the span when finishReason ===
'tool_calls' and prevents subsequent tool spans from attaching; modify the
onChunk logic in the middleware (function handling chunks / onChunk) to skip
closing the iteration span when chunk.finishReason === 'tool_calls' (i.e.,
return early or avoid span.end() and clearing state.currentIterationSpan in that
case), and instead ensure the iteration span is closed from onToolPhaseComplete
after tools finish (use onToolPhaseComplete to call span.end() and clear
state.currentIterationSpan), so onBeforeToolCall can attach tool spans to the
still-open state.currentIterationSpan.
---
Duplicate comments:
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 555-565: Before ending a dangling iteration span in the block that
checks state.currentIterationSpan, first iterate and close any remaining tool
spans in state.currentToolSpans (the spans that would normally be closed by
onAfterToolCall) so they aren't leaked; for each tool span call
safeCall('otel.onSpanEnd', () => onSpanEnd?.({ kind: 'tool', ctx, /* include any
identifier available */ }, toolSpan)) and then toolSpan.end(), clearing
state.currentToolSpans, and only after that invoke onSpanEnd for the iteration
({ kind: 'iteration', ctx, iteration: state.iterationCount - 1 }), end
state.currentIterationSpan and set it to null to ensure tool spans are ended
before the parent iteration span.
- Around line 102-109: safeCall currently swallows errors and the label, making
extension callback failures invisible; update the safeCall(label, fn)
implementation to surface failures by logging the label and the caught error
(including err.stack when available) before returning undefined (or rethrowing
if you prefer); reference the safeCall function and its parameters label and err
so you can replace the void err/void label no-ops with a meaningful call to your
logger (e.g., console.error or the project's logger) that emits both the label
and error details.
🪄 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
Run ID: b00781b7-cd2a-4d2b-ae42-560625f37df5
📒 Files selected for processing (3)
docs/advanced/otel.mdpackages/typescript/ai/src/middlewares/otel.tspackages/typescript/ai/tests/middlewares/otel.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/advanced/otel.md
Comment on lines
+243
to
+255
| if (captureContent) { | ||
| for (const sys of config.systemPrompts) { | ||
| iterSpan.addEvent('gen_ai.system.message', { | ||
| content: safeCall('otel.redact', () => redact(sys)) ?? sys, | ||
| }) | ||
| } | ||
| for (const m of config.messages) { | ||
| const body = serializeContent(m.content) | ||
| if (body.length === 0) continue | ||
| iterSpan.addEvent(messageEventName(m.role), { | ||
| content: safeCall('otel.redact', () => redact(body)) ?? body, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Fail closed when redaction or content serialization fails.
With captureContent: true, a throwing redact falls back to the raw prompt/completion/tool result. That can leak exactly the content the caller tried to redact. Also, JSON.stringify(info.result) can throw for circular or BigInt tool results and skip tool-span finalization.
Suggested hardening
const tokenHistogram = meter?.createHistogram('gen_ai.client.token.usage', {
description: 'GenAI client token usage',
unit: '{token}',
})
+ const redactForEvent = (text: string) =>
+ safeCall('otel.redact', () => redact(text)) ?? '[redaction_failed]'
+
return {
@@
for (const sys of config.systemPrompts) {
iterSpan.addEvent('gen_ai.system.message', {
- content: safeCall('otel.redact', () => redact(sys)) ?? sys,
+ content: redactForEvent(sys),
})
}
@@
if (body.length === 0) continue
iterSpan.addEvent(messageEventName(m.role), {
- content: safeCall('otel.redact', () => redact(body)) ?? body,
+ content: redactForEvent(body),
})
@@
span.addEvent('gen_ai.choice', {
- content:
- safeCall('otel.redact', () =>
- redact(state.assistantTextBuffer),
- ) ?? state.assistantTextBuffer,
+ content: redactForEvent(state.assistantTextBuffer),
})
@@
if (captureContent && state.currentIterationSpan) {
const body =
- typeof info.result === 'string'
- ? info.result
- : JSON.stringify(info.result ?? null)
+ safeCall('otel.serializeToolResult', () =>
+ typeof info.result === 'string'
+ ? info.result
+ : JSON.stringify(info.result ?? null),
+ ) ?? '[unserializable_tool_result]'
state.currentIterationSpan.addEvent('gen_ai.tool.message', {
- content: safeCall('otel.redact', () => redact(body)) ?? body,
+ content: redactForEvent(body),
tool_call_id: info.toolCallId,
})
}Also applies to: 283-289, 405-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 243 - 255, When
captureContent is true, ensure redact and serializeContent never cause raw
content to be emitted: wrap calls to redact(...) and serializeContent(...) in
try/catch (or centralize via safeCall) and on any error replace content with a
non-sensitive sentinel like "<redaction_failed>" or "<serialization_failed>"
before calling iterSpan.addEvent (use messageEventName, gen_ai.system.message,
or the tool-span events), and ensure iterSpan.addEvent is always executed so
spans are finalized; apply the same hardening to the other similar blocks
referenced (around the logic in captureContent at the locations matching 283-289
and 405-413) and reference functions used: captureContent, redact,
serializeContent, safeCall, iterSpan.addEvent, and messageEventName.
Comment on lines
+276
to
+300
| if (chunk.finishReason) { | ||
| span.setAttribute('gen_ai.response.finish_reasons', [ | ||
| chunk.finishReason, | ||
| ]) | ||
| } | ||
| if (chunk.model) span.setAttribute('gen_ai.response.model', chunk.model) | ||
|
|
||
| if (captureContent && state.assistantTextBuffer.length > 0) { | ||
| span.addEvent('gen_ai.choice', { | ||
| content: | ||
| safeCall('otel.redact', () => | ||
| redact(state.assistantTextBuffer), | ||
| ) ?? state.assistantTextBuffer, | ||
| }) | ||
| state.assistantTextBuffer = '' | ||
| } | ||
|
|
||
| safeCall('otel.onSpanEnd', () => | ||
| onSpanEnd?.( | ||
| { kind: 'iteration', ctx, iteration: state.iterationCount - 1 }, | ||
| span, | ||
| ), | ||
| ) | ||
| span.end() | ||
| state.currentIterationSpan = null |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the real hook order around model RUN_FINISHED and tool execution.
# Expected: if tool hooks happen after RUN_FINISHED, the middleware must keep the iteration span open for finishReason=tool_calls.
rg -nP -C4 'RUN_FINISHED|onBeforeToolCall|onAfterToolCall|onToolPhaseComplete|beforeTools|afterTools' packages/typescript/ai/srcRepository: TanStack/ai
Length of output: 50367
🏁 Script executed:
cat -n packages/typescript/ai/src/middlewares/otel.ts | head -400 | tail -150Repository: TanStack/ai
Length of output: 5826
🏁 Script executed:
rg -n 'handleRunFinishedEvent|executeToolCalls|afterTools|beforeTools' packages/typescript/ai/src/activities/chat/index.ts | head -30Repository: TanStack/ai
Length of output: 623
🏁 Script executed:
rg -n 'onToolPhaseComplete' packages/typescript/ai/src/middlewares/otel.tsRepository: TanStack/ai
Length of output: 37
🏁 Script executed:
sed -n '660,990p' packages/typescript/ai/src/activities/chat/index.ts | head -80Repository: TanStack/ai
Length of output: 2320
🏁 Script executed:
rg -n "finishReason.*tool_calls|tool_calls.*finishReason" packages/typescript/ai/src --max-count 10Repository: TanStack/ai
Length of output: 377
🏁 Script executed:
rg -n "type.*FinishReason|enum.*FinishReason|finishReason:" packages/typescript/ai/src/types --max-count 20Repository: TanStack/ai
Length of output: 127
🏁 Script executed:
fd -n "types.ts" packages/typescript/ai/src | head -5Repository: TanStack/ai
Length of output: 285
🏁 Script executed:
rg -n "finishReason\?" packages/typescript/ai/src -A2 -B2 | grep -E "finishReason|tool_calls|stop|length" | head -30Repository: TanStack/ai
Length of output: 538
🏁 Script executed:
sed -n '1040,1100p' packages/typescript/ai/src/activities/chat/index.tsRepository: TanStack/ai
Length of output: 2123
🏁 Script executed:
wc -l packages/typescript/ai/src/middlewares/otel.ts && tail -100 packages/typescript/ai/src/middlewares/otel.tsRepository: TanStack/ai
Length of output: 3193
🏁 Script executed:
# Double-check: verify that onToolPhaseComplete is actually called in the main loop
rg -n "runOnToolPhaseComplete|onToolPhaseComplete" packages/typescript/ai/src/activities/chat/index.tsRepository: TanStack/ai
Length of output: 216
🏁 Script executed:
sed -n '820,840p' packages/typescript/ai/src/activities/chat/index.ts && echo "---" && sed -n '980,1000p' packages/typescript/ai/src/activities/chat/index.tsRepository: TanStack/ai
Length of output: 1613
Don't close currentIterationSpan when finishReason: 'tool_calls' — tool execution happens after RUN_FINISHED.
When a model returns finishReason: 'tool_calls', the RUN_FINISHED chunk arrives first and the current onChunk hook closes the iteration span immediately (lines 299-300). Tool execution then follows, but onBeforeToolCall returns early because currentIterationSpan is already null (line 336), preventing tool spans from attaching to the iteration span.
Keep the iteration span open through the tool phase by returning early when finishReason === 'tool_calls', then close it from onToolPhaseComplete after tool execution finishes. The middleware system already calls onToolPhaseComplete after tool phases (lines 828 and 991 in index.ts).
Suggested direction
if (captureContent && state.assistantTextBuffer.length > 0) {
span.addEvent('gen_ai.choice', {
content:
safeCall('otel.redact', () =>
redact(state.assistantTextBuffer),
) ?? state.assistantTextBuffer,
})
state.assistantTextBuffer = ''
}
+ if (chunk.finishReason === 'tool_calls') {
+ return
+ }
+
safeCall('otel.onSpanEnd', () =>
onSpanEnd?.(
{ kind: 'iteration', ctx, iteration: state.iterationCount - 1 },
span,
),
)
span.end()
state.currentIterationSpan = null
})
return undefined
},
+ onToolPhaseComplete(ctx) {
+ safeCall('otel.onToolPhaseComplete', () => {
+ const state = stateByCtx.get(ctx)
+ const span = state?.currentIterationSpan
+ if (!state || !span) return
+
+ safeCall('otel.onSpanEnd', () =>
+ onSpanEnd?.(
+ { kind: 'iteration', ctx, iteration: state.iterationCount - 1 },
+ span,
+ ),
+ )
+ span.end()
+ state.currentIterationSpan = null
+ })
+ },Also applies to: 333-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 276 - 300, The
onChunk handler currently closes the iteration span (state.currentIterationSpan
= null and span.end()) whenever a chunk has a finishReason, which prematurely
closes the span when finishReason === 'tool_calls' and prevents subsequent tool
spans from attaching; modify the onChunk logic in the middleware (function
handling chunks / onChunk) to skip closing the iteration span when
chunk.finishReason === 'tool_calls' (i.e., return early or avoid span.end() and
clearing state.currentIterationSpan in that case), and instead ensure the
iteration span is closed from onToolPhaseComplete after tools finish (use
onToolPhaseComplete to call span.end() and clear state.currentIterationSpan), so
onBeforeToolCall can attach tool spans to the still-open
state.currentIterationSpan.
- Extract makeToolCall() factory in fake-otel.ts, replace 10+ as-any casts on tool-call test objects with the typed factory. - Extract runToIterationStart() helper, remove 3-line setup duplication from every behavioral test. - Use ev.runFinished() from test-utils for RUN_FINISHED chunks where the shared shorthand applies. - Cast span to FakeSpan (not any) in onSpanEnd callbacks. - Use RateLimitError subclass instead of mutating err.name via as-any. - Use as-const / satisfies instead of as-any for multimodal content arrays.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/typescript/ai/tests/middlewares/otel.test.ts (1)
410-439: Assertion is weaker than the comment suggests.The comment on line 433 says
onSpanEnd should have been called for: iteration, tool, root (in that order), but the test only locates thetoolentry and checks its capturedendedflag. If the implementation regressed to fire only one callback, or fired them in the wrong order, this test would still pass. Consider asserting on the full sequence (e.g.,expect(seen.map(s => s.kind)).toEqual(['tool', 'iteration', 'chat'])— adjust to the documented order) and that allendedflags captured at callback time arefalse. The same applies to theonAbortvariant at lines 441-468.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/tests/middlewares/otel.test.ts` around lines 410 - 439, The test "onError fires onSpanEnd for open tool spans before ending them" only checks the single 'tool' entry; update it to assert the full sequence of callbacks and that each captured span had not yet ended: after running mw.onError, assert seen.map(s => s.kind) equals the documented order (iteration, tool, root) and assert seen.every(s => s.ended === false) (also validate toolName/toolCallId on the appropriate entry). Make the same change in the corresponding onAbort test so it verifies the full sequence and that all captured ended flags are false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/typescript/ai/tests/middlewares/otel.test.ts`:
- Around line 638-654: The test currently uses expect(() => ...).not.toThrow()
which doesn't handle promise rejections from mw.onStart / mw.onFinish; change
the assertions to call those hooks and await their results (e.g., await the
possibly-promised return of otelMiddleware(...).onStart(ctx) and .onFinish(ctx,
...)) and use promise-aware expectations (await
expect(promise).resolves.not.toThrow / resolves.toBeUndefined) so rejections
fail the test, and ensure you await both hooks' completion before asserting
spans[0]!.ended; reference the otelMiddleware attributeEnricher, its onStart and
onFinish hooks, and the spans check to locate where to update the test.
---
Nitpick comments:
In `@packages/typescript/ai/tests/middlewares/otel.test.ts`:
- Around line 410-439: The test "onError fires onSpanEnd for open tool spans
before ending them" only checks the single 'tool' entry; update it to assert the
full sequence of callbacks and that each captured span had not yet ended: after
running mw.onError, assert seen.map(s => s.kind) equals the documented order
(iteration, tool, root) and assert seen.every(s => s.ended === false) (also
validate toolName/toolCallId on the appropriate entry). Make the same change in
the corresponding onAbort test so it verifies the full sequence and that all
captured ended flags are false.
🪄 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
Run ID: ed60d6b5-1f7f-44a8-9448-11f2425908e5
📒 Files selected for processing (2)
packages/typescript/ai/tests/middlewares/fake-otel.tspackages/typescript/ai/tests/middlewares/otel.test.ts
Comment on lines
+1
to
+16
| import { describe, it, expect } from 'vitest' | ||
| import { SpanKind, SpanStatusCode } from '@opentelemetry/api' | ||
| import { otelMiddleware } from '../../src/middlewares/otel' | ||
| import { | ||
| createFakeTracer, | ||
| createFakeMeter, | ||
| makeCtx, | ||
| makeToolCall, | ||
| type FakeSpan, | ||
| } from './fake-otel' | ||
| import type { | ||
| ChatMiddleware, | ||
| ChatMiddlewareContext, | ||
| ChatMiddlewareConfig, | ||
| } from '../../src/activities/chat/middleware/types' | ||
| import { ev } from '../test-utils' |
Contributor
There was a problem hiding this comment.
New ESLint errors at the import header and line 556.
Static analysis reports:
sort-imports: line 1 (expectmisordered), line 6 (createFakeMetermisordered), line 14 (ChatMiddlewareConfigmisordered).import/consistent-type-specifier-styleon line 9:type FakeSpanis an inline type specifier; it should either be hoisted into aimport typeor the whole line moved to a separate type-only import.import/orderon line 16:../test-utilsshould appear before./fake-otel.@typescript-eslint/array-typeon line 556:string[]should beArray<string>.
🧹 Suggested cleanup
-import { describe, it, expect } from 'vitest'
+import { describe, expect, it } from 'vitest'
import { SpanKind, SpanStatusCode } from '@opentelemetry/api'
import { otelMiddleware } from '../../src/middlewares/otel'
+import { ev } from '../test-utils'
import {
- createFakeTracer,
createFakeMeter,
+ createFakeTracer,
makeCtx,
makeToolCall,
- type FakeSpan,
} from './fake-otel'
+import type { FakeSpan } from './fake-otel'
import type {
ChatMiddleware,
+ ChatMiddlewareConfig,
ChatMiddlewareContext,
- ChatMiddlewareConfig,
} from '../../src/activities/chat/middleware/types'
-import { ev } from '../test-utils'
@@
- return Array.isArray(v) ? (v as string[]) : []
+ return Array.isArray(v) ? (v as Array<string>) : []Also applies to: 556-556
🧰 Tools
🪛 ESLint
[error] 1-1: Member 'expect' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 6-6: Member 'createFakeMeter' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 9-9: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
[error] 14-14: Member 'ChatMiddlewareConfig' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 16-16: ../test-utils import should occur before import of ./fake-otel
(import/order)
Critical - C1: onUsage was a no-op in production — RUN_FINISHED closed the iteration span before runOnUsage fired, dropping gen_ai.usage.* attrs and the token histogram. Fix: keep the iteration span open through tool execution and onUsage; close it on the next onConfig(beforeModel), on onFinish, or on onError/onAbort. Token histogram is also recorded directly from chunk.usage at RUN_FINISHED and redundantly from onUsage so neither hook-order variant loses data. - C3: otelMiddleware is now exported from the dedicated subpath @tanstack/ai/middlewares/otel instead of the main middlewares barrel, so importing toolCacheMiddleware or contentGuardMiddleware no longer eagerly requires @opentelemetry/api. - C4: redactor failures fail closed to the literal sentinel "[redaction_failed]" and log a warning — raw content can no longer leak when a PII redactor throws. - C2: added two middleware.spec.ts scenarios that exercise otel end-to-end through the real chat() runner (basic-text + with-tool), guarding against the C1 regression and verifying tool-span nesting. Important - I1: safeCall now logs callback failures via console.warn with a label, matching the docs' "a thrown callback becomes a log line" promise. - I2: replaced gen_ai.completion.reason='cancelled' (not a valid semconv attribute) with tanstack.ai.completion.reason. - I3: onAbort now records gen_ai.client.operation.duration with error.type='cancelled'. - I4: docs + changeset corrected — duration histogram is per-run, token histogram is per-iteration. - I5: error/abort tests now assert the full onSpanEnd sequence (iteration, tool, chat) with each span captured before .end(). - I6: tool spans still open at onFinish are swept with tanstack.ai.tool.outcome='unknown'. - I7: OtelSpanInfo is now a proper discriminated union; tool-only fields narrow inside callbacks and the internal 'as OtelSpanInfo<\"tool\">' casts are mostly gone. - I8: iteration spans now use 'chat <model> #<iteration>' so they are distinguishable in trace viewers. - I9: gen_ai.response.model dropped from the duration histogram attrs (high-cardinality). Misc - fake-otel: resolve parent via the explicit context arg passed to startSpan, eliminating the ;(span as any).parent = ... test hack. createHistogram also captures options so unit/description are assertable. - redactException paths use 'as Exception' instead of 'as Error' so non-Error throwables are preserved. - OtelSpanKind kept as a deprecated alias of OtelSpanScope to avoid shadowing OTel's built-in SpanKind. - Public types documented with JSDoc. - Assistant text buffer is now capped at maxContentLength (default 100k).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/typescript/ai/src/middlewares/otel.ts (1)
552-561:⚠️ Potential issue | 🟡 Minor
JSON.stringify(info.result)can still throw and short-circuit tool-span finalization.A tool result containing circular references or
BigIntvalues will causeJSON.stringifyto throw insideonAfterToolCall. Because this runs inside thesafeCall('otel.onAfterToolCall', …)wrapper, the throw is caught, but then everything after line 552 — including theonSpanEndcallback,toolSpan.end(), and thestate.toolSpans.delete(info.toolCallId)cleanup (lines 563-576) — is skipped, leaving the tool span dangling until theonFinish/onErrorsweep.Wrap the serialization in
safeCall(or a try/catch with a sentinel) so the span is always finalized:🛠️ Suggested change
if (captureContent && state.currentIterationSpan) { - const body = - typeof info.result === 'string' - ? info.result - : JSON.stringify(info.result ?? null) + const body = + typeof info.result === 'string' + ? info.result + : (safeCall('otel.serializeToolResult', () => + JSON.stringify(info.result ?? null), + ) ?? '[unserializable_tool_result]') state.currentIterationSpan.addEvent('gen_ai.tool.message', { content: redactContent(body), tool_call_id: info.toolCallId, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/src/middlewares/otel.ts` around lines 552 - 561, The JSON.stringify call inside otel.onAfterToolCall can throw (circular refs / BigInt) and short-circuit span finalization; update the block in onAfterToolCall where captureContent && state.currentIterationSpan is handled to perform safe serialization of info.result (either by wrapping the stringify in safeCall or a try/catch that falls back to a safe placeholder), then call redactContent and state.currentIterationSpan.addEvent with the safe string; ensure this prevents exceptions so the subsequent onSpanEnd callback, toolSpan.end(), and state.toolSpans.delete(info.toolCallId) always run (reference symbols: onAfterToolCall, captureContent, state.currentIterationSpan, info.result, redactContent, state.toolSpans.delete, toolSpan.end, onSpanEnd).
🧹 Nitpick comments (4)
packages/typescript/ai/tests/middlewares/fake-otel.ts (1)
60-130: Minor:spanContext()returns a fresh random spanId on every call.
spanContext()constructs a newspanIdviaMath.random()every invocation (line 83), so any code that captures the span context twice for the sameFakeSpanwill see two different IDs. The middleware under test doesn't seem to rely on it, but any future test that asserts span relationships viaspanContext().spanIdwould be flaky. Cache it once on creation:♻️ Suggested refactor
+ const spanId = `fake-span-${Math.random().toString(36).slice(2, 10)}` const span: FakeSpan = { name, kind: options.kind, parent, ... spanContext(): SpanContext { - return { - traceId: 'fake-trace', - spanId: `fake-span-${Math.random().toString(36).slice(2, 10)}`, - traceFlags: 1, - } + return { traceId: 'fake-trace', spanId, traceFlags: 1 } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/tests/middlewares/fake-otel.ts` around lines 60 - 130, The FakeSpan returned by makeSpan currently generates a new random spanId on every call to its spanContext() method, causing inconsistent IDs; modify makeSpan to generate and store a single spanId when the span is created (e.g., local const spanId = ... inside makeSpan) and have spanContext() return that cached spanId instead of computing a new one each time so repeated calls yield the same trace/span identifiers for the same FakeSpan.packages/typescript/ai/tests/middlewares/otel.test.ts (1)
211-223: Test would benefit from an explicit assertion that no histogram records are emitted.The "skips metrics when meter is not provided" test only asserts
onUsagedoesn't throw. It doesn't actually verify the no-meter code path. Consider asserting the record count is zero (e.g., via a spy or by checking thatcreateHistogramwas never called) so a future regression that records metrics to a null meter would fail this test rather than silently succeeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/typescript/ai/tests/middlewares/otel.test.ts` around lines 211 - 223, The test "skips metrics when meter is not provided" only ensures onUsage doesn't throw but doesn't assert no histogram metrics were recorded; update the test to spy/assert that the meter creation/recording functions are not called (e.g., ensure createHistogram was never invoked or the meter spy has zero records) when calling otelMiddleware({ tracer }) using the existing helpers (createFakeTracer, runToIterationStart, makeCtx, and mw.onUsage). Locate the test case and add an expectation that the histogram/meter spy's call count is zero so any regression that emits metrics without a meter will fail.testing/e2e/src/lib/otel-capture.ts (1)
62-76: Minor:Object.assignpatch replaces, not merges, nested fields.
Object.assign(existing, entry.patch)shallowly replaces fields. If a future caller ever passespatch: { events: [...] }orpatch: { attributes: { x: 1 } }expecting a merge, they'll silently overwrite the existing arrays/objects. Today all callers pre-merge before patching (e.g.attributes: { ...attrs }inapi.middleware-test.ts), so this is just a landmine for future changes. A short doc comment onrecordOtelSpanstating "patches are shallow; callers must pre-merge arrays/records" would prevent future footguns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/src/lib/otel-capture.ts` around lines 62 - 76, Add a brief doc comment above the recordOtelSpan function explaining that the incoming patch (entry.patch) is applied with a shallow Object.assign and therefore will replace nested objects/arrays rather than merge them; explicitly state that callers must pre-merge nested objects/arrays (e.g., attributes, events) before passing a patch. Reference recordOtelSpan, the entry.patch shape, and the use of Object.assign(existing, entry.patch) so maintainers see exactly where the shallow behavior occurs; optionally note bucketFor and CapturedSpan for context. Do not change runtime behavior in this change—only add the documentation comment.testing/e2e/tests/middleware.spec.ts (1)
99-113: Iteration span discrimination by presence oftanstack.ai.iterationis fragile.Both the chat root span and iteration spans carry
gen_ai.operation.name === 'chat'; you distinguish them by!('tanstack.ai.iteration' in s.attributes). If the middleware ever tags the root span withtanstack.ai.iterations(plural, which it does at line 764-767 ofotel.ts) and a future change accidentally uses the singular name on the root, this filter silently flips. Consider adding a stronger discriminator — e.g. asserting the chat span'snamestarts withchatand has no#suffix, or matching onSpanKind.INTERNALvsSpanKind.CLIENT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/e2e/tests/middleware.spec.ts` around lines 99 - 113, The test in middleware.spec.ts currently discriminates the root chat span from iteration spans by checking absence of the 'tanstack.ai.iteration' attribute on items in capture.spans, which is fragile; update the test to use a stronger discriminator (e.g., assert the root chat span's span.name starts with "chat " and does not include a "#" suffix, or check span.kind === SpanKind.INTERNAL vs SpanKind.CLIENT) when locating chatSpan and when filtering iterationSpans, and keep the existing asserts that spans have ended; refer to capture.spans, the 'gen_ai.operation.name' attribute check, and the 'tanstack.ai.iteration' attribute in the test and consider SpanKind or span.name patterns to reliably separate root vs iteration spans (note otel.ts sets plural 'tanstack.ai.iterations' on the root).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 409-433: The token histogram is being recorded twice for
RUN_FINISHED chunks because onChunk records metrics when chunk.usage exists and
onUsage (invoked via runOnUsage for RUN_FINISHED) records them again; to fix,
stop recording histogram metrics in onChunk (keep only span.setAttributes for
chunk.usage) and rely on onUsage to call tokenHistogram.record once, i.e.,
remove or guard the tokenHistogram.record calls in the onChunk logic that checks
chunk.usage so only onUsage performs histogram recording for token metrics
(references: onChunk, onUsage, tokenHistogram, chunk.usage, RUN_FINISHED,
runOnUsage).
In `@testing/e2e/src/routes/api.middleware-test.ts`:
- Around line 260-275: The GET handler currently returns in-memory captures via
getOtelCapture(testId) to any caller; to fix, gate both the GET handler and the
companion POST 'otel' handling logic by checking an explicit env flag (e.g.,
process.env.E2E_TEST === '1') at the top of those handlers and return a 404/403
(or similar non-revealing response) when the flag is not set; optionally also
add an Origin/CORS check if you prefer stricter runtime guarding, but the
primary change is to wrap the GET (and POST otel) code paths with the env check
to prevent accidental exposure in non-test builds.
- Around line 10-23: Reorder the imports in
testing/e2e/src/routes/api.middleware-test.ts so ESLint import/order and
sort-imports pass: move the value import "import { z } from 'zod'" above the
type import from '@opentelemetry/api', and inside the type import list ensure
AttributeValue appears before Attributes (i.e., sort the named type imports:
AttributeValue, Attributes, Context, Histogram, MetricOptions, Meter, Span,
SpanContext, SpanStatus, Tracer) so the OTel type import and the zod import are
in the correct group/order.
- Around line 134-139: The fake span's isRecording() logic should mirror real
OTel by returning false once end() is called; declare a boolean flag (e.g., let
ended = false) alongside the existing status variable, update end() (the
function that calls recordOtelSpan(captureId, { id, patch: { ended: true } }))
to set ended = true when invoked, and change isRecording() to return !ended (so
it only returns true until end() is called), leaving status/SpanStatusCode.UNSET
semantics untouched.
In `@testing/e2e/tests/middleware.spec.ts`:
- Around line 93-94: Protect against undefined testId and failed GET by
validating testId before constructing captureUrl and by checking response.ok
before calling response.json; specifically, in the otel tests that build
captureUrl (using testId and captureUrl) ensure you either skip or fail fast if
testId is falsy (so you don't send ?testId=undefined) and then after the
page.request.get call check response.ok and throw or log a helpful error
containing response.status/text before calling response.json; update both
occurrences that use page.request.get/captureUrl and the subsequent
response.json() call (and any waitForFunction relying on the request) so a
missing testId or non-OK response produces an immediate, clear failure instead
of a timeout.
---
Duplicate comments:
In `@packages/typescript/ai/src/middlewares/otel.ts`:
- Around line 552-561: The JSON.stringify call inside otel.onAfterToolCall can
throw (circular refs / BigInt) and short-circuit span finalization; update the
block in onAfterToolCall where captureContent && state.currentIterationSpan is
handled to perform safe serialization of info.result (either by wrapping the
stringify in safeCall or a try/catch that falls back to a safe placeholder),
then call redactContent and state.currentIterationSpan.addEvent with the safe
string; ensure this prevents exceptions so the subsequent onSpanEnd callback,
toolSpan.end(), and state.toolSpans.delete(info.toolCallId) always run
(reference symbols: onAfterToolCall, captureContent, state.currentIterationSpan,
info.result, redactContent, state.toolSpans.delete, toolSpan.end, onSpanEnd).
---
Nitpick comments:
In `@packages/typescript/ai/tests/middlewares/fake-otel.ts`:
- Around line 60-130: The FakeSpan returned by makeSpan currently generates a
new random spanId on every call to its spanContext() method, causing
inconsistent IDs; modify makeSpan to generate and store a single spanId when the
span is created (e.g., local const spanId = ... inside makeSpan) and have
spanContext() return that cached spanId instead of computing a new one each time
so repeated calls yield the same trace/span identifiers for the same FakeSpan.
In `@packages/typescript/ai/tests/middlewares/otel.test.ts`:
- Around line 211-223: The test "skips metrics when meter is not provided" only
ensures onUsage doesn't throw but doesn't assert no histogram metrics were
recorded; update the test to spy/assert that the meter creation/recording
functions are not called (e.g., ensure createHistogram was never invoked or the
meter spy has zero records) when calling otelMiddleware({ tracer }) using the
existing helpers (createFakeTracer, runToIterationStart, makeCtx, and
mw.onUsage). Locate the test case and add an expectation that the
histogram/meter spy's call count is zero so any regression that emits metrics
without a meter will fail.
In `@testing/e2e/src/lib/otel-capture.ts`:
- Around line 62-76: Add a brief doc comment above the recordOtelSpan function
explaining that the incoming patch (entry.patch) is applied with a shallow
Object.assign and therefore will replace nested objects/arrays rather than merge
them; explicitly state that callers must pre-merge nested objects/arrays (e.g.,
attributes, events) before passing a patch. Reference recordOtelSpan, the
entry.patch shape, and the use of Object.assign(existing, entry.patch) so
maintainers see exactly where the shallow behavior occurs; optionally note
bucketFor and CapturedSpan for context. Do not change runtime behavior in this
change—only add the documentation comment.
In `@testing/e2e/tests/middleware.spec.ts`:
- Around line 99-113: The test in middleware.spec.ts currently discriminates the
root chat span from iteration spans by checking absence of the
'tanstack.ai.iteration' attribute on items in capture.spans, which is fragile;
update the test to use a stronger discriminator (e.g., assert the root chat
span's span.name starts with "chat " and does not include a "#" suffix, or check
span.kind === SpanKind.INTERNAL vs SpanKind.CLIENT) when locating chatSpan and
when filtering iterationSpans, and keep the existing asserts that spans have
ended; refer to capture.spans, the 'gen_ai.operation.name' attribute check, and
the 'tanstack.ai.iteration' attribute in the test and consider SpanKind or
span.name patterns to reliably separate root vs iteration spans (note otel.ts
sets plural 'tanstack.ai.iterations' on the root).
🪄 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
Run ID: 59575d7e-e478-4302-a25f-a17a7c219b8d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/otel-middleware.mddocs/advanced/otel.mdpackages/typescript/ai/package.jsonpackages/typescript/ai/src/middlewares/index.tspackages/typescript/ai/src/middlewares/otel.tspackages/typescript/ai/tests/middlewares/fake-otel.tspackages/typescript/ai/tests/middlewares/otel.test.tspackages/typescript/ai/vite.config.tstesting/e2e/package.jsontesting/e2e/src/lib/otel-capture.tstesting/e2e/src/routes/api.middleware-test.tstesting/e2e/src/routes/middleware-test.tsxtesting/e2e/tests/middleware.spec.ts
✅ Files skipped from review due to trivial changes (6)
- packages/typescript/ai/src/middlewares/index.ts
- testing/e2e/src/routes/middleware-test.tsx
- packages/typescript/ai/vite.config.ts
- testing/e2e/package.json
- docs/advanced/otel.md
- .changeset/otel-middleware.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/typescript/ai/package.json
- otel.ts: remove token histogram recording from onChunk; the chat runner always follows RUN_FINISHED-with-usage by runOnUsage, so recording in both hooks double-counted every histogram. onChunk keeps attribute setting, onUsage is the canonical histogram source. - otel.ts: wrap JSON.stringify in onAfterToolCall with safeCall. A tool result containing circular refs or BigInt used to throw out of the handler body and skip toolSpan.end() + state.toolSpans cleanup, leaving the span dangling. Failed serialization now yields a sentinel string and the handler always finalizes the span. - fake-otel.ts: cache spanId once per FakeSpan so repeat spanContext() calls return a consistent id. - api.middleware-test.ts (e2e): gate both the POST 'otel' middleware mode and the GET capture fetch behind an OTEL_TEST_ENABLED env check so the endpoint cannot be used as an oracle outside E2E runs. Track 'ended' separately so the capture span's isRecording() flips after end() like real OTel. Reorder imports. - otel-capture.ts: document the shallow Object.assign patch behavior on recordOtelSpan so future callers know nested fields replace instead of merging. - middleware.spec.ts: discriminate root-vs-iteration spans by SpanKind (INTERNAL vs CLIENT) instead of presence of the 'tanstack.ai.iteration' attribute; factor the capture-fetch into a helper that validates testId and response.ok.
@opentelemetry/api is an intentional optional peer dependency of @tanstack/ai — it is referenced from src/middlewares/otel.ts but users who don't import the otel subpath never load it. Knip's default rule flags referenced optional peers as an error; add a workspace-scoped ignoreDependencies entry so knip stays happy without forcing the peer to be non-optional.
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.
Summary
otelMiddleware({ tracer, meter?, ... })factory in@tanstack/ai/middlewares.@opentelemetry/apiadded as an optional peer dependency.Test plan
pnpm --filter @tanstack/ai test:lib— 15 new middleware tests green (fake Tracer/Meter coverage across spans, metrics, content capture, error/abort, extension points, concurrent isolation).pnpm --filter @tanstack/ai test:types/test:eslint— green.@opentelemetry/sdk-node) before merge.Summary by CodeRabbit