feat(ai-semantic-conventions, instrumentation-anthropic): align GenAI semconv with OTel 1.40 and migrate Anthropic instrumentation#896
Conversation
dvirski
changed the title
dvirski
changed the title
dvirski
requested review from
OzBenSimhonTraceloop,
galkleinman,
galzilber and
nina-kollman
|
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:
📝 WalkthroughWalkthroughBumps Opentelemetry and Anthropic SDK versions, adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AnthropicSDK as Anthropic SDK
participant TraceloopInst as Traceloop Instrumentation
participant Utils as instrumentation-utils
participant OTel as OpenTelemetry Span/Exporter
Client->>AnthropicSDK: invoke chat/completion API
AnthropicSDK-->>TraceloopInst: SDK callback/hook (response, stop_reason, tokens, content blocks)
TraceloopInst->>Utils: formatInputMessages / formatOutputMessage / mapAnthropicContentBlock
Utils-->>TraceloopInst: JSON-encoded message parts & mapped finish_reason
TraceloopInst->>OTel: start/span attributes (provider, operation, input/output messages, tokens, finish reasons)
OTel-->>OTel: record span and export to backend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ai-semantic-conventions/package.json (1)
38-38:⚠️ Potential issue | 🟠 MajorUpdate
@opentelemetry/semantic-conventionsdependency to align with v1.40.0 migration.The package.json still depends on
^1.38.0, but the CHANGELOG documents migration to v1.40.0 and the migration helper explicitly references v1.40.0 attributes likeATTR_GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENSthat don't exist in v1.38.0. Update the dependency to^1.40.0to provide access to these new attributes.Suggested fix
"dependencies": { "@opentelemetry/api": "^1.9.0", - "@opentelemetry/semantic-conventions": "^1.38.0" + "@opentelemetry/semantic-conventions": "^1.40.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ai-semantic-conventions/package.json` at line 38, The package.json dependency for `@opentelemetry/semantic-conventions` is out of date (currently "^1.38.0") but the migration and helper reference v1.40.0 attributes (e.g., ATTR_GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS); update the version specifier to "^1.40.0" in packages/ai-semantic-conventions package.json so the referenced constants are available at runtime.packages/instrumentation-anthropic/src/instrumentation.ts (1)
321-355:⚠️ Potential issue | 🟠 MajorStreaming chat spans drop
stop_reason.In the
message_deltacase,result.stop_reasonis never updated, somessages.stream()spans never emitATTR_GEN_AI_RESPONSE_FINISH_REASONS. The non-streaming path does, so the two APIs now disagree for the same response metadata.Suggested fix
case "message_delta": if (chunk.usage) { Object.assign(result.usage, chunk.usage); } + result.stop_reason = + chunk.delta?.stop_reason ?? result.stop_reason; + result.stop_sequence = + chunk.delta?.stop_sequence ?? result.stop_sequence; break;Based on learnings: Instrumentations must extract request/response data and token usage from wrapped calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/src/instrumentation.ts` around lines 321 - 355, The streaming path for GEN_AI_OPERATION_NAME_VALUE_CHAT builds a result Message but never updates result.stop_reason in the message_delta branch, so spans from messages.stream() never emit ATTR_GEN_AI_RESPONSE_FINISH_REASONS; modify the stream handling in instrumentation.ts (inside the GEN_AI_OPERATION_NAME_VALUE_CHAT branch where result is created and the for-await loop handles chunk types) to set result.stop_reason when present on streaming chunks (e.g., in the "message_delta" case, do result.stop_reason = chunk.stop_reason if chunk.stop_reason is defined) and similarly ensure any "message_end"/"message_start" handling preserves/propagates stop_reason so the final result contains stop_reason for downstream span emission.
🧹 Nitpick comments (2)
packages/instrumentation-anthropic/package.json (1)
48-49: Consider adding peerDependencies for@anthropic-ai/sdk.The Anthropic SDK is only in
devDependenciesfor testing. Without apeerDependenciesdeclaration, consumers could use SDK versions incompatible with the instrumentation's patching logic, potentially causing silent failures at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/package.json` around lines 48 - 49, Add a peerDependencies entry for "@anthropic-ai/sdk" in packages/instrumentation-anthropic/package.json to declare the supported SDK range (while keeping the existing "@anthropic-ai/sdk" in devDependencies for tests); update peerDependencies to a sensible semver range that matches the versions your instrumentation patches (e.g., the minimum supported version up through the latest tested major), and document this constraint in package.json's metadata so consumers installing the instrumentation get a compatibility warning if their SDK version is incompatible.packages/instrumentation-anthropic/test/instrumentation.test.ts (1)
141-143: Assert the normalized finish reason, not just its presence.These checks still pass if the instrumentation leaks the raw Anthropic reason instead of the standardized one. Since finish-reason normalization is part of the behavior under test, please lock the expected value down here.
Suggested fix
- assert.ok(Array.isArray(finishReasons), "finish_reasons should be an array"); - assert.ok(finishReasons.length > 0, "finish_reasons should not be empty"); + assert.deepStrictEqual(finishReasons, ["stop"]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/test/instrumentation.test.ts` around lines 141 - 143, The test currently only checks presence of finish_reasons; update it to assert the normalized value rather than just existence: read chatSpan.attributes[`${ATTR_GEN_AI_RESPONSE_FINISH_REASONS}`] (finishReasons) and assert finishReasons[0] equals the canonical normalized finish reason (e.g. "stop") or the project constant representing that value (use the exported constant if available), so the test verifies normalization from Anthropic raw reasons to the standardized finish-reason.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/agents/code-reviewer.md:
- Around line 20-23: The instructions incorrectly state that `git diff` shows
"staged + unstaged" changes; update the documentation lines that list the git
commands so they accurately describe their behavior: change the description for
`git diff` to say it shows unstaged changes only, and ensure `git diff --cached`
(or `git diff --staged`) is documented as showing staged changes; edit the list
item text (the lines containing "Run `git diff`..." and "Run `git diff
--cached`...") to reflect this correction so readers won't miss staged-only
changes.
In `@CLAUDE.md`:
- Around line 142-156: The fenced code block in CLAUDE.md (the directory tree
block starting with "instrumentation-[provider]/") is unlabeled and triggers
markdownlint MD040; update that block to include a language specifier (for
example change the opening fence to ```text) so the block is explicitly labeled
and the lint warning is resolved.
- Around line 217-219: Update the onboarding step that points contributors to
the wrong initialization file: change step 6 to reference the SDK's tracing
initialization module (the tracing index used to register instrumentation
providers) instead of the node-server-sdk file so contributors are directed to
the tracing initialization code where providers are actually registered.
In `@packages/ai-semantic-conventions/CHANGELOG.md`:
- Line 23: Remove the stray empty markdown list item represented by a lone "-"
in CHANGELOG.md (the incomplete list entry) by either deleting that line
entirely or replacing it with a completed changelog entry text describing the
change; ensure the surrounding list formatting remains valid and runs as a
proper bullet item if you add content.
- Line 14: Replace the placeholder issue link "[`#xxx`]" in the CHANGELOG entry
"**semantic-conventions:** Migrate to `@opentelemetry/semantic-conventions`
v1.40.0" with the actual issue or PR number (e.g. "[`#1234`]") and update the URL
to point to the real GitHub issue/PR
(https://github.com/traceloop/openllometry-js/issues/<number>) so the markdown
link is correct and not left as a placeholder.
In `@packages/ai-semantic-conventions/src/message-formatters.ts`:
- Around line 39-56: The formatter formatInputMessages currently replaces every
array-based content block with {type: "text", content: JSON.stringify(block)},
which loses original block types for gen_ai.input.messages; update
formatInputMessages so that when message.content is an array you preserve each
block's existing type and content (i.e., if block.type exists, keep block.type
and block.content as-is) and only default to {type: "text", content: ...} for
raw strings or blocks without a type (ensuring text blocks have string content,
serializing only when necessary), rather than unconditionally JSON.stringify
every block.
- Around line 22-31: formatSystemInstructions currently collapses an array input
into a single text block and JSON-encodes inner objects; change it so when the
system argument is an array you return a JSON string of the array mapped into
structured blocks (preserve each element's type and fields if it's an object,
and convert plain string items to { type: "text", content: ... }) instead of
wrapping the whole array into one { type: "text" } entry; update logic in
formatSystemInstructions to detect Array input, map elements accordingly
(preserve object entries, convert strings to text blocks) and then
JSON.stringify the resulting array so gen_ai.system_instructions remains
structured and not double-encoded.
In
`@packages/ai-semantic-conventions/src/semantic-conventions-migration-helper.ts`:
- Around line 1-3: The file
packages/ai-semantic-conventions/src/semantic-conventions-migration-helper.ts is
a temporary migration helper that must be removed before merging; delete this
file from the branch, or if the content needs to be preserved, move the
commented migration notes into a documentation location (e.g., project README or
an ADR) and remove the TODO and commented paragraphs from the source tree so no
temporary helper remains in main.
In `@packages/ai-semantic-conventions/src/SemanticAttributes.ts`:
- Around line 135-139: The LLMRequestTypeValues enum was repurposed to hold
gen_ai operation-name constants; instead restore LLMRequestTypeValues to the
legacy vocabulary values (the original LLM_REQUEST_TYPE semantics) and add a new
exported enum (e.g., GenAIOperationNameValues or GenAiOperationNameValues) that
contains the GEN_AI_OPERATION_NAME_* constants
(GEN_AI_OPERATION_NAME_VALUE_TEXT_COMPLETION, GEN_AI_OPERATION_NAME_VALUE_CHAT,
etc.); update any internal imports/usages (e.g., Bedrock/Cohere snippets) to
import the new GenAiOperationNameValues when they need op-name values while
leaving existing callers of LLMRequestTypeValues unchanged.
---
Outside diff comments:
In `@packages/ai-semantic-conventions/package.json`:
- Line 38: The package.json dependency for `@opentelemetry/semantic-conventions`
is out of date (currently "^1.38.0") but the migration and helper reference
v1.40.0 attributes (e.g., ATTR_GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS); update
the version specifier to "^1.40.0" in packages/ai-semantic-conventions
package.json so the referenced constants are available at runtime.
In `@packages/instrumentation-anthropic/src/instrumentation.ts`:
- Around line 321-355: The streaming path for GEN_AI_OPERATION_NAME_VALUE_CHAT
builds a result Message but never updates result.stop_reason in the
message_delta branch, so spans from messages.stream() never emit
ATTR_GEN_AI_RESPONSE_FINISH_REASONS; modify the stream handling in
instrumentation.ts (inside the GEN_AI_OPERATION_NAME_VALUE_CHAT branch where
result is created and the for-await loop handles chunk types) to set
result.stop_reason when present on streaming chunks (e.g., in the
"message_delta" case, do result.stop_reason = chunk.stop_reason if
chunk.stop_reason is defined) and similarly ensure any
"message_end"/"message_start" handling preserves/propagates stop_reason so the
final result contains stop_reason for downstream span emission.
---
Nitpick comments:
In `@packages/instrumentation-anthropic/package.json`:
- Around line 48-49: Add a peerDependencies entry for "@anthropic-ai/sdk" in
packages/instrumentation-anthropic/package.json to declare the supported SDK
range (while keeping the existing "@anthropic-ai/sdk" in devDependencies for
tests); update peerDependencies to a sensible semver range that matches the
versions your instrumentation patches (e.g., the minimum supported version up
through the latest tested major), and document this constraint in package.json's
metadata so consumers installing the instrumentation get a compatibility warning
if their SDK version is incompatible.
In `@packages/instrumentation-anthropic/test/instrumentation.test.ts`:
- Around line 141-143: The test currently only checks presence of
finish_reasons; update it to assert the normalized value rather than just
existence: read chatSpan.attributes[`${ATTR_GEN_AI_RESPONSE_FINISH_REASONS}`]
(finishReasons) and assert finishReasons[0] equals the canonical normalized
finish reason (e.g. "stop") or the project constant representing that value (use
the exported constant if available), so the test verifies normalization from
Anthropic raw reasons to the standardized finish-reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a88e515-e2bd-45ef-bc08-60d01c04bd47
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.claude/agents/code-reviewer.md.gitignoreCLAUDE.mdpackages/ai-semantic-conventions/CHANGELOG.mdpackages/ai-semantic-conventions/package.jsonpackages/ai-semantic-conventions/src/SemanticAttributes.tspackages/ai-semantic-conventions/src/index.tspackages/ai-semantic-conventions/src/message-formatters.tspackages/ai-semantic-conventions/src/semantic-conventions-migration-helper.tspackages/instrumentation-anthropic/package.jsonpackages/instrumentation-anthropic/src/instrumentation.tspackages/instrumentation-anthropic/test/instrumentation.test.tspackages/sample-app/package.jsonpackages/sample-app/src/sample_anthropic.tspackages/traceloop-sdk/tsconfig.json
💤 Files with no reviewable changes (1)
- .gitignore
There was a problem hiding this comment.
Do we need this in the repo? Either way, shouldn't be in the scope of this PR IMO
There was a problem hiding this comment.
Agree, @dvirski please migrate it into a dedicated pr 🙏
There was a problem hiding this comment.
removed this from the pr
|
|
||
| LLM_REQUEST_TYPE: "llm.request.type", | ||
| LLM_USAGE_TOTAL_TOKENS: "llm.usage.total_tokens", | ||
| // Replaced with: OTel ATTR_GEN_AI_OPERATION_NAME - "gen_ai.operation.name" |
There was a problem hiding this comment.
This comment is misplaced
There was a problem hiding this comment.
llm.request.type is actually gen_ai.operation.name now
Those comments are there until we complete all js instrumentations.
Added todos to mark them to be removed before parent branch is merged.
There was a problem hiding this comment.
removed this anyway
| VECTOR_DB_GET_INCLUDE_VALUES: "db.vector.get.include_values", | ||
|
|
||
| // LLM Workflows | ||
| VECTOR_DB_VENDOR: "db.system", // Already aligned with OTel - ATTR_DB_SYSTEM - "db.system" |
There was a problem hiding this comment.
All of those will be removed. there are now todos there
| COMPLETION = GEN_AI_OPERATION_NAME_VALUE_TEXT_COMPLETION, | ||
| CHAT = GEN_AI_OPERATION_NAME_VALUE_CHAT, |
There was a problem hiding this comment.
Isn't the Idea to align and use Gen_AI constant as is, not remap them?
There was a problem hiding this comment.
This was reverted, since other instrumentations use it as legacy
dvirski
changed the title
| appName: "sample_anthropic", | ||
| apiKey: process.env.TRACELOOP_API_KEY, | ||
| disableBatch: true, | ||
| instrumentModules: { |
There was a problem hiding this comment.
We should remove this.
The sample app is like our non official documentation. We do not want to show to use only the Anthropic module.
There was a problem hiding this comment.
The instrumentation doesn't work without it.
Anyway this is an example for anthropic.
There was a problem hiding this comment.
The semantic convention folder should only have convention consts.
We need to find another place for the formatters.
There was a problem hiding this comment.
I put it there since It's coupled to semantic conventions - The formatters produce JSON strings that conform to the OpenTelemetry semantic conventions format ( gen_ai.input.messages, gen_ai.output.messages)
If not there I can create a utils package under /packages
What do you say?
There was a problem hiding this comment.
It should be a util package
There was a problem hiding this comment.
Moved it to util package under packages
| GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS: | ||
| "gen_ai.usage.cache_creation_input_tokens", | ||
|
|
||
| GEN_AI_USAGE_CACHE_CREATION_INPUT_TOKENS: "gen_ai.usage.cache_creation_input_tokens", |
There was a problem hiding this comment.
If this is exists in the official Otel then we should replace it to that and delete this one.
There was a problem hiding this comment.
This is kept until we migrate all instrumentations.
Marked in todo for removal
|
|
||
| GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS: "gen_ai.usage.cache_read_input_tokens", | ||
| GEN_AI_USAGE_REASONING_TOKENS: "gen_ai.usage.reasoning_tokens", | ||
| // Replaced with: OTel ATTR_GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS - "gen_ai.usage.cache_read.input_tokens" |
There was a problem hiding this comment.
If this is exists in the official Otel then we should replace it to that and delete this one.
There was a problem hiding this comment.
This is kept until we migrate all instrumentations.
Marked in todo for removal
| // Replaced with: OTel ATTR_GEN_AI_USAGE_CACHE_READ_INPUT_TOKENS - "gen_ai.usage.cache_read.input_tokens" | ||
| // Kept for backwards compatibility | ||
|
|
||
| GEN_AI_USAGE_REASONING_TOKENS: "gen_ai.usage.reasoning_tokens", // No OTel equivalent yet - keep as custom |
There was a problem hiding this comment.
No need for comments as we are keeping this file to only hold attributes that does not exists yet
| GEN_AI_USAGE_REASONING_TOKENS: "gen_ai.usage.reasoning_tokens", // No OTel equivalent yet - keep as custom | |
| GEN_AI_USAGE_REASONING_TOKENS: "gen_ai.usage.reasoning_tokens" |
There was a problem hiding this comment.
This is kept for other developers (lena) to easily understand what's happening and to be able to migrated fast.
Added todo to remove all unnecessary comments after we update all instrumentations and before merge of the parent branch.
There was a problem hiding this comment.
Removed all unnecessary comments
Code reviewFound 2 issues:
openllmetry-js/packages/ai-semantic-conventions/src/SemanticAttributes.ts Lines 135 to 140 in 7ad7207 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
|
|
There was a problem hiding this comment.
Minor naming suggestion: SemanticAttributes.ts feels a bit out of place in this package.
Most files here use kebab-case (message-formatters.ts, semantic-conventions-migration-helper.ts), so this filename looks inconsistent with the rest of the project style. I think a kebab-case name would fit better.
Would semantic-attributes.ts make more sense here?
OzBenSimhonTraceloop
left a comment
OzBenSimhonTraceloop
left a comment
There was a problem hiding this comment.
please add to Anthropic CHANGELOG.MD a full and detailed explanation contains exactly whats changed, and mention its a breaking change.
There was a problem hiding this comment.
Agree, @dvirski please migrate it into a dedicated pr 🙏
| @@ -0,0 +1,63 @@ | |||
| // This paragraph is a helper for migrating to open telemetry semantic conventions version 1.40 | |||
|
|
||
| export enum LLMRequestTypeValues { | ||
| COMPLETION = "completion", | ||
| COMPLETION = "text_completion", |
There was a problem hiding this comment.
This is a breaking change, whom ever need to use text_completion should use Otel GEN_AI_OPERATION_NAME_VALUE_TEXT_COMPLETION
There was a problem hiding this comment.
yeah, changed that back, since cohere and bedrock still uses this.
|
|
||
| const thinkingBlock = content.find( | ||
| const thinkingBlock = outputMessages[0].parts.find( | ||
| (block: ContentBlock) => block.type === "thinking", |
There was a problem hiding this comment.
| (block: ContentBlock) => block.type === "thinking", | |
| (block: ContentBlock) => block.type === "reasoning", |
There was a problem hiding this comment.
the fact this wan't failing got me notice that we never add here recording and these tests aren't running, so please fix it as well 🙏 🎖️
There was a problem hiding this comment.
Fixed and checked locally that tests fail with "thinking" and pass with "reasoning"
| chroma.log | ||
|
|
||
| # claude | ||
| .claude |
There was a problem hiding this comment.
Not relevant for this PR
|
|
||
| ## [0.23.0](https://github.com/traceloop/openllmetry-js/compare/v0.22.5...v0.23.0) (2026-03-18) | ||
|
|
||
| ### ⚠ BREAKING CHANGES |
There was a problem hiding this comment.
its not breaking change, please add exactly whats added, be very specific so it will be clear
There was a problem hiding this comment.
reverted all changelog.md changes. since they will be overwritten anyway by lerna on release.
| /** | ||
| * Maps a single Anthropic content block to its OTel-compliant part object. | ||
| * Used by both formatInputMessages and formatOutputMessage to avoid duplication. | ||
| */ | ||
| function mapContentBlock(block: any): object { |
There was a problem hiding this comment.
if this logic is relevant to Anthropic only then move it to there
There was a problem hiding this comment.
I created instrumentation-utils minimal package. It now has:
content-block-mappers.ts - this file will hold each instrumentation specific block mappers, since they differ.
The mapping objects will be passed to message formatters from each instrumentation.
message-formatters.ts - shared formatters generic logic for formatting:
gen_ai.system_instructions
gen_ai.input.messages
gen_ai.output.messages
dvirski
changed the title
dvirski
force-pushed
the
dvir/tlp-1933-anthropic-instumentation-semconv-alignment
branch
2 times, most recently
from
c7769c1 to
8566600
Compare
…mantic-conventions with OpenTelemetry 1.40 semantic conventions+ Migrate Anthropic Instrumentation to OpenTelemetry 1.40 semantic conventions
dvirski
force-pushed
the
dvir/tlp-1933-anthropic-instumentation-semconv-alignment
branch
from
8566600 to
ea0a981
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/instrumentation-anthropic/src/instrumentation.ts (1)
324-385:⚠️ Potential issue | 🟠 MajorExtract
stop_reasonfrommessage_delta.deltaand handle non-text content blocks in chat streaming.The chat streaming path (lines 324–384) only extracts
chunk.usagefrommessage_deltaevents but ignoreschunk.delta.stop_reason. Anthropic's streaming API sends the final stop reason in themessage_deltaevent'sdeltaobject (e.g.,{"type":"message_delta","delta":{"stop_reason":"end_turn"}}), but the current code never updatesresult.stop_reasonin the chat case. Additionally,content_block_deltaonly handlestext_delta; it skipstool_input_delta(for tool-use blocks) andthinking_delta(for extended thinking). This causes_endSpanto emit incomplete telemetry: missingATTR_GEN_AI_RESPONSE_FINISH_REASONSand incompletegen_ai.output.messages. The completion streaming path correctly extractschunk.stop_reason(line 400–401); apply the same pattern to chat'smessage_deltahandler and extend content block reconstruction to cover all delta types.
🧹 Nitpick comments (1)
packages/instrumentation-anthropic/test/instrumentation.test.ts (1)
192-205: Add streamed finish-reason assertions here.This test only verifies that
partsis an array. A concrete assertion onATTR_GEN_AI_RESPONSE_FINISH_REASONSandoutputMessages[0].finish_reasonwould catch regressions in the new streaming output path much earlier.As per coding guidelines,
Instrumentations must extract request/response data and token usage from wrapped calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/test/instrumentation.test.ts` around lines 192 - 205, Add assertions to verify streamed finish reasons: parse outputMessages from chatSpan as already done, then assert that chatSpan.attributes[ATTR_GEN_AI_RESPONSE_FINISH_REASONS] exists and contains the expected finish reason(s) (e.g., "stop" or the expected streamed reason), and also assert outputMessages[0].finish_reason equals the same expected value. Update the test around parsing outputMessages (the block using outputMessages and chatSpan) to include checks that +chatSpan.attributes[`${ATTR_GEN_AI_RESPONSE_FINISH_REASONS}`] is present and that outputMessages[0].finish_reason strictly matches the expected finish reason to catch streaming regressions.
🤖 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/instrumentation-utils/src/content-block-mappers.ts`:
- Around line 56-62: The document branch currently uses block.source?.id and a
hardcoded PDF MIME type; update the case "document" branch in
content-block-mappers.ts to read the file identifier from block.source?.file_id,
remove the invalid fallback to block.source?.url, and branch on
block.source?.type ("text" | "base64" | "file") to set file_id and mime_type
correctly: for "text" use the file_id and set mime_type to "text/plain" (or
block.source?.media_type if present), for "base64" use the file_id and use the
provided block.source?.media_type, and for "file" use the file_id but set
mime_type to null/undefined (since the Files API doesn't expose it); ensure the
returned object keys remain type, modality, mime_type, and file_id.
---
Nitpick comments:
In `@packages/instrumentation-anthropic/test/instrumentation.test.ts`:
- Around line 192-205: Add assertions to verify streamed finish reasons: parse
outputMessages from chatSpan as already done, then assert that
chatSpan.attributes[ATTR_GEN_AI_RESPONSE_FINISH_REASONS] exists and contains the
expected finish reason(s) (e.g., "stop" or the expected streamed reason), and
also assert outputMessages[0].finish_reason equals the same expected value.
Update the test around parsing outputMessages (the block using outputMessages
and chatSpan) to include checks that
+chatSpan.attributes[`${ATTR_GEN_AI_RESPONSE_FINISH_REASONS}`] is present and
that outputMessages[0].finish_reason strictly matches the expected finish reason
to catch streaming regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d30f56a-7747-48f1-836f-cc90506b0082
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/ai-semantic-conventions/package.jsonpackages/ai-semantic-conventions/src/SemanticAttributes.tspackages/instrumentation-anthropic/package.jsonpackages/instrumentation-anthropic/src/instrumentation.tspackages/instrumentation-anthropic/test/instrumentation.test.tspackages/instrumentation-utils/package.jsonpackages/instrumentation-utils/rollup.config.jspackages/instrumentation-utils/src/content-block-mappers.tspackages/instrumentation-utils/src/index.tspackages/instrumentation-utils/src/message-formatters.tspackages/sample-app/package.jsonpackages/sample-app/src/sample_anthropic.tspackages/traceloop-sdk/tsconfig.json
✅ Files skipped from review due to trivial changes (5)
- packages/traceloop-sdk/tsconfig.json
- packages/ai-semantic-conventions/package.json
- packages/sample-app/package.json
- packages/sample-app/src/sample_anthropic.ts
- packages/instrumentation-utils/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/instrumentation-anthropic/package.json
- packages/ai-semantic-conventions/src/SemanticAttributes.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/instrumentation-utils/src/content-block-mappers.ts (1)
85-87: Minor: Document URL source hardcodesapplication/pdfmime_type.Line 87 hardcodes
mime_type: "application/pdf"for URL-sourced documents. While Anthropic currently primarily uses URL sources for PDFs, this assumption may not hold if the API expands URL support to other document types.Consider whether passthrough without
mime_type(like thefilecase) would be more future-proof, or if this is acceptable given current API constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-utils/src/content-block-mappers.ts` around lines 85 - 87, The URL branch in content-block-mappers.ts currently hardcodes mime_type to "application/pdf" in the case "url" return object; update the case "url" in the mapper to avoid assuming PDF by either (a) omitting mime_type (match the `file` case behavior) or (b) if the source provides a mime/type field (e.g., src.mime_type or similar), pass that through into the returned object instead of the hardcoded value so the return becomes { type: "uri", uri: src.url } or { type: "uri", mime_type: src.mime_type, uri: src.url } depending on availability.packages/instrumentation-anthropic/test/content-block-mapper.test.ts (1)
19-25: Use package imports instead of cross-package relative paths.Cross-package relative imports break package abstraction and prevent independent consumption. Import from
@traceloop/instrumentation-utilsinstead, which properly exports these functions:Proposed fix
-import { mapAnthropicContentBlock } from "../../instrumentation-utils/src/content-block-mappers"; -import { - formatSystemInstructions, - formatInputMessages, - formatInputMessagesFromPrompt, - formatOutputMessage, -} from "../../instrumentation-utils/src/message-formatters"; +import { + mapAnthropicContentBlock, + formatSystemInstructions, + formatInputMessages, + formatInputMessagesFromPrompt, + formatOutputMessage, +} from "@traceloop/instrumentation-utils";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/test/content-block-mapper.test.ts` around lines 19 - 25, Replace cross-package relative imports with the public package export: import mapAnthropicContentBlock, formatSystemInstructions, formatInputMessages, formatInputMessagesFromPrompt, and formatOutputMessage from "@traceloop/instrumentation-utils" instead of "../../instrumentation-utils/src/..." so the test uses the package boundary; locate the import statements referencing mapAnthropicContentBlock and the four formatter functions and update them to import from "@traceloop/instrumentation-utils".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/instrumentation-anthropic/test/content-block-mapper.test.ts`:
- Around line 19-25: Replace cross-package relative imports with the public
package export: import mapAnthropicContentBlock, formatSystemInstructions,
formatInputMessages, formatInputMessagesFromPrompt, and formatOutputMessage from
"@traceloop/instrumentation-utils" instead of
"../../instrumentation-utils/src/..." so the test uses the package boundary;
locate the import statements referencing mapAnthropicContentBlock and the four
formatter functions and update them to import from
"@traceloop/instrumentation-utils".
In `@packages/instrumentation-utils/src/content-block-mappers.ts`:
- Around line 85-87: The URL branch in content-block-mappers.ts currently
hardcodes mime_type to "application/pdf" in the case "url" return object; update
the case "url" in the mapper to avoid assuming PDF by either (a) omitting
mime_type (match the `file` case behavior) or (b) if the source provides a
mime/type field (e.g., src.mime_type or similar), pass that through into the
returned object instead of the hardcoded value so the return becomes { type:
"uri", uri: src.url } or { type: "uri", mime_type: src.mime_type, uri: src.url }
depending on availability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6faaa17c-2bf8-4015-a39e-c6adbdc543f1
📒 Files selected for processing (5)
packages/instrumentation-anthropic/src/instrumentation.tspackages/instrumentation-anthropic/test/content-block-mapper.test.tspackages/instrumentation-anthropic/test/mapper-fixtures.jsonpackages/instrumentation-utils/src/content-block-mappers.tspackages/instrumentation-utils/tsconfig.json
✅ Files skipped from review due to trivial changes (3)
- packages/instrumentation-utils/tsconfig.json
- packages/instrumentation-anthropic/test/mapper-fixtures.json
- packages/instrumentation-anthropic/src/instrumentation.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/instrumentation-anthropic/test/content-block-mapper.test.ts (2)
33-201: Consider adding edge case tests for unknown source types.The test coverage for
mapAnthropicContentBlockis comprehensive for documented block types. However, based on the implementation incontent-block-mappers.ts, there are fallback paths for unknown source types withinimageanddocumentblocks that aren't exercised:
- Image with unknown source type — when
src.typeis neither"base64"nor"url"- Document with unknown source type — when
src.typefalls through the switch defaultThese fallbacks return
{ type: block.type, ...block }as a GenericPart.🧪 Suggested additional edge case tests
it("maps image with unknown source type as GenericPart", () => { const block = { type: "image", source: { type: "unknown_source", foo: "bar" }, }; const result = mapAnthropicContentBlock(block); assert.deepStrictEqual(result, { type: "image", source: { type: "unknown_source", foo: "bar" } }); }); it("maps document with unknown source type as GenericPart", () => { const block = { type: "document", source: { type: "unknown_source", data: "xyz" }, }; const result = mapAnthropicContentBlock(block); assert.deepStrictEqual(result, { type: "document", source: { type: "unknown_source", data: "xyz" } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/test/content-block-mapper.test.ts` around lines 33 - 201, Add edge-case tests exercising the fallback branches in mapAnthropicContentBlock for image and document blocks with unknown source types: create one test that calls mapAnthropicContentBlock with an image block where source.type is an unexpected string (e.g., "unknown_source") and assert it returns the GenericPart (i.e., the original block shape), and a second test that does the same for a document block with an unknown source.type; reference the mapAnthropicContentBlock function and assert deep equality with the input block preserved.
329-348: Add tests formax_tokensandstop_sequencefinish reason mappings.The
anthropicFinishReasonMap(from context snippet 4) defines 4 mappings, but only 2 are tested:
Anthropic Value Mapped Value Tested? end_turnstop✓ max_tokenslength✗ stop_sequencestop✗ tool_usetool_call✓ Adding tests for
max_tokensandstop_sequencewould ensure the finish reason map is fully verified, especially sincemax_tokens → lengthis a distinct mapping.🧪 Suggested additional tests
it("maps stop_reason via finishReasonMap — max_tokens becomes length", () => { const result = JSON.parse( formatOutputMessage(fixtures.outputMessages.text_only, "max_tokens", anthropicFinishReasonMap, GEN_AI_OPERATION_NAME_VALUE_CHAT, mapAnthropicContentBlock) ); assert.strictEqual(result[0].finish_reason, "length"); }); it("maps stop_reason via finishReasonMap — stop_sequence becomes stop", () => { const result = JSON.parse( formatOutputMessage(fixtures.outputMessages.text_only, "stop_sequence", anthropicFinishReasonMap, GEN_AI_OPERATION_NAME_VALUE_CHAT, mapAnthropicContentBlock) ); assert.strictEqual(result[0].finish_reason, "stop"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/instrumentation-anthropic/test/content-block-mapper.test.ts` around lines 329 - 348, Test coverage is missing for two entries in anthropicFinishReasonMap; add two unit tests (using formatOutputMessage, mapAnthropicContentBlock and fixtures.outputMessages) that assert "max_tokens" maps to "length" and "stop_sequence" maps to "stop" (parallel to the existing "end_turn" and "tool_use" tests) so the finishReason mapping is fully verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/instrumentation-anthropic/test/content-block-mapper.test.ts`:
- Around line 33-201: Add edge-case tests exercising the fallback branches in
mapAnthropicContentBlock for image and document blocks with unknown source
types: create one test that calls mapAnthropicContentBlock with an image block
where source.type is an unexpected string (e.g., "unknown_source") and assert it
returns the GenericPart (i.e., the original block shape), and a second test that
does the same for a document block with an unknown source.type; reference the
mapAnthropicContentBlock function and assert deep equality with the input block
preserved.
- Around line 329-348: Test coverage is missing for two entries in
anthropicFinishReasonMap; add two unit tests (using formatOutputMessage,
mapAnthropicContentBlock and fixtures.outputMessages) that assert "max_tokens"
maps to "length" and "stop_sequence" maps to "stop" (parallel to the existing
"end_turn" and "tool_use" tests) so the finishReason mapping is fully verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a03c735-f4b3-4a49-b4e8-7001fc9601d5
📒 Files selected for processing (1)
packages/instrumentation-anthropic/test/content-block-mapper.test.ts
| case "message_delta": | ||
| if (chunk.usage) { | ||
| Object.assign(result.usage, chunk.usage); | ||
| } | ||
| break; |
There was a problem hiding this comment.
| case "message_delta": | |
| if (chunk.usage) { | |
| Object.assign(result.usage, chunk.usage); | |
| } | |
| if (chunk.delta?.stop_reason) { | |
| result.stop_reason = chunk.delta.stop_reason; | |
| } | |
| break; |
just random Claude finding not made by u, the handler copies chunk.usage but never reads chunk.delta.stop_reason, so streaming spans are missing gen_ai.response.finish_reasons
There was a problem hiding this comment.
Yes, those stream fixes I will open in a different PR
| type: "text", | ||
| text: current.text + chunk.delta.text, | ||
| citations: current.citations, | ||
| }; |
There was a problem hiding this comment.
Another claude finding here, the handler accumulates text_delta chunks but not thinking_delta chunks, so streaming with extended thinking produces empty reasoning parts. Its an easy fix, we can just add else if
else if (
current.type === "thinking" &&
chunk.delta.type === "thinking_delta"
) {
(current as any).thinking =
((current as any).thinking || "") + chunk.delta.thinking;
}
Consider adding tests for that as I don't think we have good coverage for thinking
OzBenSimhonTraceloop
changed the title
6 participants
Summary by CodeRabbit
New Features
Tests
Chores