Skip to content

Conversation

@Sai-Tom
Copy link

@Sai-Tom Sai-Tom commented Dec 4, 2025

Summary

The OpenAI SDK returns APIPromise objects with additional methods like .withResponse().
The previous async/await instrumentation converted these to regular Promises, losing those methods.

This PR uses Proxy + handleCallbackErrors pattern (matching anthropic-ai) to preserve the original return type.

Changes

  • Use Proxy instead of async function wrapper
  • Use handleCallbackErrors for non-streaming responses (preserves Promise subclass)
  • Add handleStreamingError helper for consistency
  • Add tests for APIPromise.withResponse() preservation

Test Plan

  • yarn build:dev passes
  • yarn test passes
  • yarn lint passes
  • Added tests for APIPromise preservation

The OpenAI SDK returns APIPromise objects with additional methods like .withResponse(). The previous async/await instrumentation converted these to regular Promises, losing those methods.

Use Proxy + handleCallbackErrors pattern (matching anthropic-ai) to preserve the original return type.
  - Use startInactiveSpan instead of startSpan/startSpanManual because they internally use handleCallbackErrors which calls .then() on Promises, creating a new instance and losing APIPromise methods
  - Add try-catch for synchronous exceptions
  - Add tests for error handling (sync throw + async reject)
  - Update tests to match real OpenAI SDK behavior"
- Use startInactiveSpan instead of startSpan/startSpanManual because they internally use handleCallbackErrors which calls .then() on Promises, creating a new instance and losing APIPromise methods
- Add try-catch for synchronous exceptions
- Use .finally() to ensure span always ends even if attribute processing throws
- Add tests for error handling (sync throw + async reject)
- Update tests to match real OpenAI SDK behavior
@AbhiPrasad AbhiPrasad requested a review from RulaKhaled December 4, 2025 17:38
Comment on lines +202 to +210
const span = startInactiveSpan({
name: `${operationName} ${model}`,
op: getSpanOperation(methodPath),
attributes: requestAttributes as Record<string, SpanAttributeValue>,
});

if (options.recordInputs && params) {
addRequestAttributes(span, params);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unhandled exceptions from addRequestAttributes at line 209 can prevent spans from ending, leading to incomplete tracing.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

The addRequestAttributes function, called at line 209, is not wrapped in a try-catch block. This function internally calls getTruncatedJsonString(), which uses JSON.stringify(). JSON.stringify() can throw exceptions, for example, when encountering circular references or BigInt values without a replacer. If such an exception occurs, it propagates out of the Proxy's apply handler, preventing the span created at line 202 from being properly ended. This violates the span's lifecycle and leads to incomplete tracing data, contrasting with the protected addResponseAttributes call at line 238.

💡 Suggested Fix

Wrap the call to addRequestAttributes(span, params) at line 209 in a try-catch block to gracefully handle potential errors, mirroring the error handling for addResponseAttributes.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/core/src/tracing/openai/index.ts#L202-L210

Potential issue: The `addRequestAttributes` function, called at line 209, is not wrapped
in a try-catch block. This function internally calls `getTruncatedJsonString()`, which
uses `JSON.stringify()`. `JSON.stringify()` can throw exceptions, for example, when
encountering circular references or `BigInt` values without a replacer. If such an
exception occurs, it propagates out of the Proxy's `apply` handler, preventing the span
created at line 202 from being properly ended. This violates the span's lifecycle and
leads to incomplete tracing data, contrasting with the protected `addResponseAttributes`
call at line 238.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5640211


if (options.recordInputs && params) {
addRequestAttributes(span, params);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Unprotected addRequestAttributes call can leave span open

The addRequestAttributes call on line 209 is outside the try/catch block that surrounds target.apply(). If addRequestAttributes throws an exception (e.g., from JSON.stringify failing on circular references in user-provided params), the span created on line 202 will never be ended - it's started but the span.end() calls only happen inside the try/catch error handler or the promise's finally() block. In the previous implementation, this call was inside the try/catch. This could cause orphaned spans and potential memory leaks.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant