-
Notifications
You must be signed in to change notification settings - Fork 345
the loader is broken for openai agents, adds the shim for it as well #6347
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: master
Are you sure you want to change the base?
Conversation
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 the PR! Without looking deeply into it, this seems correct.
Would you mind adding some tests for it as well? That would be very helpful!
I found a second exclude list and added it to that as well -- we weren't using the initialize.mjs @BridgeAR I did a little looking, where are the tests for these, I can't find them? happy to add a test |
cde496d
to
9ba06ed
Compare
c33bb39
to
c3d064b
Compare
I was told I had to push a signed commit so I did that above as well |
We'd need a new integration test that uses some of the parts that should be ignored. You can have a look at the tests that were introduced in the PR that introduced the ignore list: https://github.com/DataDog/dd-trace-js/pull/5267/files#diff-f6d5f969d408b29ba2d5f3c988e30ff11cc978c464289d6c1f0866633980b38a :) So it's pretty much about adding a simple example application of openai that would currently fail to our tests. If it's instrumented without error, and we receive spans, we'd know it works fine. |
yea I can do this, basically after we installed the openai-agents package the app stopped booting so such a test should be simple enough |
49b7e6f
to
af80ae3
Compare
af80ae3
to
a00a56d
Compare
@BridgeAR @Kyle-Verhoog I added a test, this lib as far as I can tell isn't actually instrumented (except incidentally) so the only real relevant test was whether the app can start with it installed. This was the issue it was preventing loading by monkeypatching. I just added it in the sandbox for the openai test and it fails to load up without my changes, but with them does load properly |
@gmmeyer thank you, that already looks quite good. The additional packages may also be included specifically for the test by using our externals.json file. I am going to pick it up to finish the PR. |
What does this PR do?
This adds the OpenAI shim for agents to the ignore list
Motivation
The loader registration is broken for openai agents and gives this error when you try to use it (I changed this to purely the relative path)
It seems like we are overriding this improperly. You can see this error is coming from here:
https://github.com/openai/openai-agents-js/blob/f075ca780d3ecdb7e1fb737f863ca378f76d3b29/packages/agents-core/src/config.ts#L3-L15
Meaning that somehwere in this file:
https://github.com/openai/openai-agents-js/blob/f075ca780d3ecdb7e1fb737f863ca378f76d3b29/packages/agents-core/src/shims/shims-node.ts#L15
we are removing the loadenv improperly. It also seems like we are ignoring the shims for the other openai package, so I figured this simply needed to be added here as well.
(I never touched this when I was at datadog so all of this is a bit of a guess on what we should do, but if I do not do this my app doesn't boot)
Plugin Checklist
Additional Notes