fix(ai-sdk): model provider attribute transformation for openai#609
fix(ai-sdk): model provider attribute transformation for openai#609galkleinman merged 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 3ccdc9f in 51 seconds. Click for details.
- Reviewed
81lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:177
- Draft comment:
Delegating transformation to transformAiSdkSpan() improves modularity. Ensure that this function fully replicates all the previous inline transformations and error handling (e.g., JSON parsing in ai.prompt.messages). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests ensuring that a function replicates previous behavior, which violates the rule against asking the PR author to ensure behavior is intended or tested. It doesn't provide a specific code suggestion or ask for a specific test to be written.
2. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:161
- Draft comment:
The JSDoc comment still mentions 'Vercel AI compatibility'. Update it to reflect the more general AI SDK span transformations now applied. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/src/lib/tracing/span-processor.ts:170
- Draft comment:
Legacy inline transformation logic for Vercel AI Adapters was removed. Confirm that transformAiSdkSpan now handles all attribute mappings (e.g., span name remapping, response text and prompt message adjustments, token usage calculations) as before. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_wbhErfoXRRs3Nvg1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 40c6ea4 in 1 minute and 47 seconds. Click for details.
- Reviewed
662lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/tracing/ai-sdk-transformations.ts:48
- Draft comment:
Suggestion: In transformPromptMessages, the catch block silently swallows JSON parsing errors. Consider logging the error for easier debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a data transformation layer where parsing errors likely indicate malformed input data. The current approach of silently skipping seems intentional - it allows the transformation to continue with other attributes even if one fails. Adding logging could create noise in production logs since this isn't necessarily an error condition. The comment is about code quality but may not be a clear improvement. The suggestion could help with debugging issues in development. Silent failures can make issues harder to track down. However, this appears to be an intentional design choice to gracefully handle malformed data without disrupting the flow. Logging every parse failure could create unnecessary noise for what is essentially a non-critical transformation step. The current silent failure approach appears to be an intentional design choice for graceful degradation. Adding logging would be a matter of preference rather than a clear improvement.
2. packages/traceloop-sdk/test/ai-sdk-transformations.test.ts:339
- Draft comment:
Testing Improvement: Consider adding tests for calculateTotalTokens with token values of 0 to ensure the sum is computed correctly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While testing zero values is generally good practice, in this case the function is just doing basic addition. The existing tests already cover type conversion and edge cases. Zero is not a special case for addition - if the function works for 50+25, it will work for 0+0. The function is simple enough that zero values don't introduce any special behavior. I could be underestimating the importance of explicitly testing boundary values like zero. There could be type coercion issues specific to zero in JavaScript. The existing test with string values already verifies type coercion behavior. Zero addition is a fundamental operation that doesn't need special testing beyond normal values. The comment should be deleted as the suggested test would be redundant given the existing comprehensive test coverage.
Workflow ID: wflow_q5nfLC0RIbBWwHwv
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| const promptTokens = attributes[`${SpanAttributes.LLM_USAGE_PROMPT_TOKENS}`]; | ||
| const completionTokens = attributes[`${SpanAttributes.LLM_USAGE_COMPLETION_TOKENS}`]; | ||
|
|
||
| if (promptTokens && completionTokens) { |
There was a problem hiding this comment.
Bug: The condition in calculateTotalTokens uses 'if (promptTokens && completionTokens)' which fails when token values are 0 (a valid case). Consider checking for undefined (e.g., promptTokens != null && completionTokens != null) instead.
| if (promptTokens && completionTokens) { | |
| if (promptTokens != null && completionTokens != null) { |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ad0afa6 in 1 minute and 6 seconds. Click for details.
- Reviewed
661lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/traceloop-sdk/src/lib/tracing/ai-sdk-transformations.ts:81
- Draft comment:
Potential issue: using a truthy check for promptTokens and completionTokens may skip valid 0 values. Consider checking for undefined instead, e.g. (promptTokens !== undefined && completionTokens !== undefined). - Reason this comment was not posted:
Comment was on unchanged code.
2. packages/traceloop-sdk/src/lib/tracing/ai-sdk-transformations.ts:88
- Draft comment:
Consider validating that the 'vendor' is a string before calling startsWith to avoid runtime errors if an unexpected type is provided. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_LIWhVBhgT7VeBXTi
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Refactor AI SDK span transformations into
ai-sdk-transformations.tsand add comprehensive tests.transformAiSdkSpaninai-sdk-transformations.tsto handle AI SDK span transformations.span-processor.tstoai-sdk-transformations.ts.onSpanEndinspan-processor.tsusingtransformAiSdkSpan.transformAiSdkSpanNamechanges span names for specific AI operations.transformResponseText,transformPromptMessages,transformPromptTokens,transformCompletionTokens,calculateTotalTokens, andtransformVendorhandle specific attribute transformations.ai-sdk-transformations.test.tswith tests for all transformation functions, ensuring correct behavior and handling of edge cases.This description was created by
for ad0afa6. You can customize this summary. It will automatically update as commits are pushed.