-
Notifications
You must be signed in to change notification settings - Fork 324
feat: openai-java v3.0+ instrumentation #9959
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
…stead of forcing it
…d "streamed async request completion test with withRawResponse"
Call decorateWithResponse from the wrappers
Rename span resources to be aligned with trace-py Add http.client resource assertion
Test case renaming
Reorder tests by synch, async
Fix Embeddings fixture for the latestDepTest when base64
…wrappers and instrumentation.
…s use ListWriter, and LLMObsSpanMapper is never reached.
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
PerfectSlayer
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.
First round of comments for the core part. I will let IDM review the instrumentation part.
| * <blockquote> | ||
| * <p><b>NOTE:</b> The order matters. If the muzzle check fails with a NoClassDefFoundError | ||
| * (as seen in build/reports/muzzle-*.txt), it may be because some helper classes depend on | ||
| * each other. In this case, the order must be adjusted accordingly. | ||
| * </blockquote> |
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.
❔ question: Which order are you referring here? Helper orders but there is no helper. Or instrumentation order?
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 returned helper class names are in order. For example, if you return an array containing "Foo" and "Bar," and "Foo" references "Bar," then it will fail with a NoClassDefFoundError, which might not be obvious since both are declared.
| assert value =~ expected: "Tag \"$name\": \"${value.toString()}\" does not match pattern \"$expected\"" | ||
| } else if (expected instanceof Class) { | ||
| assert ((Class) expected).isInstance(value): "Tag \"$name\": instance check $expected failed for \"${value.toString()}\" of class \"${value.class}\"" | ||
| assert ((Class) expected).isInstance(value): "Tag \"$name\": instance check $expected failed for \"${value.toString()}\" of class \"${value?.class}\"" |
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.
🔨 issue: Is that to prevent null value? If so, how the value.toString() did not crash earlier?
| @@ -0,0 +1,25 @@ | |||
| apply from: "$rootDir/gradle/java.gradle" | |||
| apply plugin: 'idea' | |||
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.
❔ question: What's the need for the idea plugin?
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.
I don't know. Some modules have it. Is it no longer needed?
|
|
||
| private CharSequence llmObsSpanName(CoreSpan<?> span) { | ||
| CharSequence operationName = span.getOperationName(); | ||
| CharSequence resourceName = span.getResourceName(); |
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.
🔍 nitpick: Resource name can only be fetched in case of OpenAI
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.
Correct. I'll move this
| this.contentLength = req.contentLength | ||
| this.contentType = req.contentType?.split(";") | ||
| this.body = req.inputStream.bytes | ||
| this.method = req.method |
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.
❔ question: Is this useful? It's only used by OpenAI instrumentation tests and has only POST from their records.
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.
It's useful for distinguishing request files with different HTTP methods but the same body. However, it can be removed since they are always POST.
What Does This Do
Adds openai-java v3.0+ instrumentation for completions, chat completions, embeddings, and responses.
APM shared tests:
LLMObs shared tests 14*/15:
test_chat_completion_telemetryFAILED because the shared test expects a namespace for the entire batch, but it should allow the namespace to be part of the metric data. Since Java batches telemetry data across subsystems and cannot guarantee that all the data belongs to the same namespace.Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: AIDM-163