-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): Preserve OpenAI APIPromise methods like withResponse() #18402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
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.
packages/core/test/tracing/openai-integration-functions.test.ts
Outdated
Show resolved
Hide resolved
- 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
| const span = startInactiveSpan({ | ||
| name: `${operationName} ${model}`, | ||
| op: getSpanOperation(methodPath), | ||
| attributes: requestAttributes as Record<string, SpanAttributeValue>, | ||
| }); | ||
|
|
||
| if (options.recordInputs && params) { | ||
| addRequestAttributes(span, params); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
RulaKhaled
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for contributing! I have a question
| // We use startInactiveSpan instead of startSpan/startSpanManual because those | ||
| // internally use handleCallbackErrors which calls .then() on Promises, creating | ||
| // a new Promise instance and losing APIPromise's custom methods like .withResponse(). | ||
| const span = startInactiveSpan({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using start span here?
I understand that your purpose here is to use handleCallbackErrors and handle promises, you can still achieve this with start span, and as long as we didn't end the span in a different scope/context, it should work fine.
Summary
The OpenAI SDK returns
APIPromiseobjects with additional methods like.withResponse().The previous
async/awaitinstrumentation converted these to regular Promises, losing those methods.This PR uses
Proxy+handleCallbackErrorspattern (matchinganthropic-ai) to preserve the original return type.Changes
Proxyinstead ofasync functionwrapperhandleCallbackErrorsfor non-streaming responses (preserves Promise subclass)handleStreamingErrorhelper for consistencyAPIPromise.withResponse()preservationTest Plan
yarn build:devpassesyarn testpassesyarn lintpasses