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

fix(openai): add structured output instrumentation #2111

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

9dogs
Copy link
Contributor

@9dogs 9dogs commented Oct 10, 2024

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Fixes #1815. I'm not sure if the response_format span attribute should be added in this PR as well.

Also, the metrics tests are a bit fragile since each executed test adds its metrics to the reader.get_metrics_data() result, so figuring out what was added is a bit tricky.

@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 10, 2024
@CLAassistant
Copy link

CLAassistant commented Oct 10, 2024

CLA assistant check
All committers have signed the CLA.

@dosubot dosubot bot added python Pull requests that update Python code testing labels Oct 10, 2024
@nirga nirga changed the title feat(openai): add structured output instrumentation fix(openai): add structured output instrumentation Oct 10, 2024
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks @9dogs! We add the response format here or in a separate PR - up to you (if you do that - we might as well add everything from the recently published semantic conventions)

@9dogs
Copy link
Contributor Author

9dogs commented Oct 11, 2024

Thanks @9dogs! We add the response format here or in a separate PR - up to you (if you do that - we might as well add everything from the recently published semantic conventions)

I cannot say I fully understand what's going on with semconvs, so let's leave the scope of this PR as it is.

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks @9dogs! Left a small comment

@@ -197,6 +197,32 @@ def _instrument(self, **kwargs):
"Assistants.create",
assistants_create_wrapper(tracer),
)
wrap_function_wrapper(
Copy link
Member

Choose a reason for hiding this comment

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

@9dogs we should wrap these in try-except since this may fail on old OpenAI SDK versions (this is why the tests are currently failing)

Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

Thanks @9dogs!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 15, 2024
@nirga nirga merged commit 5e709c3 into traceloop:main Oct 15, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer python Pull requests that update Python code size:XL This PR changes 500-999 lines, ignoring generated files. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: support structured outputs for OpenAI
3 participants