-
Notifications
You must be signed in to change notification settings - Fork 20
switch to 2.18.1, and switch openai to upstream #763
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
Conversation
🔍 Preview links for changed docs |
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.
Just some nits
The idea is that the issue will be solved, right? Otherwise, we might have to mark the update as a breaking change where relevant... |
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
Co-authored-by: Fabrizio Ferri-Benedetti <[email protected]>
This will be a permanent breaking change. If we merge this, I'll open PRs for the docs for the next version |
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.
LGTM, I think we just need to have an issue that describes the follow-up tasks for the removal of openai.
// OTEL_INSTRUMENTATION_OPENAI=false | ||
// OTEL_INSTRUMENTATION_OPENAI_CLIENT=true |
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.
What is the difference between those two variables ? are there two instrumentations or at least two config options in upstream ?
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.
the upstream one uses OTEL_INSTRUMENTATION_OPENAI, while our implementation uses OTEL_INSTRUMENTATION_OPENAI_CLIENT. If the upstream one had used the same var, it wouldn't have been a breaking change and we could have just removed our implementation
import org.junit.jupiter.api.Test; | ||
import org.junit.jupiter.api.extension.RegisterExtension; | ||
|
||
@Disabled |
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.
If I understand correctly disabling those tests is required because now the upstream instrumentation is used. Do we already have an issue that describes the deprecation and removal steps ? Aligning the removal with the update to 3.x upstream seems a good option as it already forces to do a new major.
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.
No issue opened yet, I will open when this is merged
upstream agent now 2.18.1 (needs some small internal test changes)
this distributions openai-client is turned off, leaving the upstream openai instrumentation to do the work (needed a couple of tests disabled)