Skip to content
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

handles _thenUnwrap for OpenAI's custom APIPromise class #5233

Closed

Conversation

josemussa
Copy link
Contributor

@josemussa josemussa commented Feb 7, 2025

What does this PR do?

Tracing is failing for requests that include a call to the OpenAI this.client.beta.chat.completions.parse method.

This happens because OpenAI uses a custom promise class. The Datadog team is already aware of this issue, as it was previously addressed for the OpenAI this.client.chat.completions.create method. I'm reusing that logic to resolve the current issue.

This PR implements the same unwrap logic and response handler that is used for the parse method, but for the _thenUnwrap method.

Motivation

When using the OpenAI Node SDK and calling await this.client.beta.chat.completions.parse, traces completely break.

This happens because OpenAI uses a custom promise. This issue was previously fixed for this.client.chat.completions.create (the non-beta version). To address this, I abstracted that fix and applied it to _thenUnwrap.

The issue is reported here openai/openai-node#1307

Plugin Checklist

Additional Notes

@josemussa josemussa requested a review from a team as a code owner February 7, 2025 22:49
@josemussa josemussa requested review from a team as code owners February 8, 2025 12:38
@josemussa josemussa changed the title Draft: handles _thenUnwrap for OpenAI's custom APIPromise class handles _thenUnwrap for OpenAI's custom APIPromise class Feb 8, 2025
@josemussa
Copy link
Contributor Author

Hi @sabrenner, I saw that you’re the main author of this file—would you be open to reviewing this PR when you have a moment? Thanks a lot!

@sabrenner
Copy link
Collaborator

Hi @josemussa! Yes, I'll be taking a look at this today, thanks for the ping!

@sabrenner
Copy link
Collaborator

This change LGTM, although I'll refactor the test a bit. Unfortunately CI for community PRs isn't great here haha - I will cherry pick your commits into a new PR to run against CI and merge 🙌

just also wanna note as an FYI that since it's technically on the beta client, we generally won't be as quick to update support if its behavior changes at all from what we assume, and once the functionality that is supported here (I'm guessing structured output) is passed onto the official client, we'll advertise tracing through the official client instead.

@josemussa
Copy link
Contributor Author

Thanks, @sabrenner! Appreciate you handling the CI situation and the refactor. Let me know if anything else is needed! 🚀

@sabrenner
Copy link
Collaborator

Hi @josemussa, the PR with the cherry picks has been merged! We should be tracing those beta chat completions now. Will follow up with the release it lands in (it might not be the next patch release, but probably 5.38.0 instead)

@josemussa
Copy link
Contributor Author

Great news! Thank you @sabrenner 🙇

@josemussa josemussa deleted the feat/fix-openai-thenunwrap branch February 20, 2025 08:10
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.

2 participants