-
Notifications
You must be signed in to change notification settings - Fork 979
Add initial instrumentation of OpenAI client #14221
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
@@ -5,6 +5,7 @@ plugins { | |||
} | |||
|
|||
dependencies { | |||
compileOnly("org.scala-lang:scala-library:2.11.12") |
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.
IntelliJ was having trouble syncing the project without this. It seemed harmless so hopefully it's ok.
🔧 The result from spotlessApply was committed to the PR branch. |
@SuppressWarnings("IdentifierName") // Want to match library's convention | ||
public class OpenAIInstrumentationModule extends InstrumentationModule { | ||
public OpenAIInstrumentationModule() { | ||
super("openai", "openai-java", "openai-java-1.1"); |
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.
Got this lint error though it looks matching to me
Expected to find super\(\n? *"openai-java",\n? *"openai-java-1.1" in instrumentation/openai/openai-java-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/openai/v1_1/OpenAIInstrumentationModule.java
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'd guess that it wants to see openai-java
as the first instrumentation name to match with the directory name.
…metry-java-instrumentation into openai-instrumentation
a302aef
to
1a238e0
Compare
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.
Was a bit scared but could somehow get the shading working
exclude(dependency("org.junit.platform:junit-platform-commons")) | ||
exclude(dependency("org.ow2.asm:asm")) | ||
// Exclude dependencies bundled during Armeria shading | ||
exclude(dependency("com.fasterxml.jackson.core:jackson-annotations")) |
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.
This is a bit weird, I wonder if instead of x-shaded-for-testing
we should have a single shaded-test-deps
project combining them? For this PR keeping changes to existing at a minimal though
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 agree that having a single shaded artifact might be better
|
||
// Ensures tests are not affected by wiremock dependencies. Wiremock itself is | ||
// safe with its original name. | ||
relocate("com.google.common", "io.opentelemetry.testing.internal.guava") |
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 was tedious to find all these, but I couldn't find a better way. I thought it could be nice to just relocate everything, but I couldn't see any opt-out in that case to not relocate slf4j / junit. So opted-in with a big list instead.
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.
Any notes to add on regenerate/check assumptions on this when libs update?
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.
Worth citing where this is coming from (albeit adapted?) Can help build confidence vs bedrock which was built from scratch and not formerly released elsewhere.
} else if (content.isArrayOfContentParts()) { | ||
return joinContentParts(content.asArrayOfContentParts()); | ||
} else { | ||
throw new IllegalStateException("Unhandled content type for " + content); |
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 assume this wont crash the actual request..
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.
Made it safer
throw new IllegalStateException("Unhandled content type for " + content); | ||
} | ||
} | ||
|
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.
Guessing we aren't lucky enough to have base types to address the duplication that way.. and as the client is generated typical issue I suppose
|
||
// Ensures tests are not affected by wiremock dependencies. Wiremock itself is | ||
// safe with its original name. | ||
relocate("com.google.common", "io.opentelemetry.testing.internal.guava") |
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.
Any notes to add on regenerate/check assumptions on this when libs update?
closing and reopening to re-trigger checks |
instrumentation/openai/openai-java-1.1/javaagent/build.gradle.kts
Outdated
Show resolved
Hide resolved
...java/io/opentelemetry/javaagent/instrumentation/openai/v1_1/OpenAIClientInstrumentation.java
Outdated
Show resolved
Hide resolved
...java/io/opentelemetry/javaagent/instrumentation/openai/v1_1/OpenAIClientInstrumentation.java
Outdated
Show resolved
Hide resolved
instrumentation/openai/openai-java-1.1/library/build.gradle.kts
Outdated
Show resolved
Hide resolved
...y/src/main/java/io/opentelemetry/instrumentation/openai/v1_1/ChatCompletionEventsHelper.java
Outdated
Show resolved
Hide resolved
...y/src/main/java/io/opentelemetry/instrumentation/openai/v1_1/ChatCompletionEventsHelper.java
Outdated
Show resolved
Hide resolved
...-1.1/library/src/main/java/io/opentelemetry/instrumentation/openai/v1_1/OpenAITelemetry.java
Outdated
Show resolved
Hide resolved
exclude(dependency("org.junit.platform:junit-platform-commons")) | ||
exclude(dependency("org.ow2.asm:asm")) | ||
// Exclude dependencies bundled during Armeria shading | ||
exclude(dependency("com.fasterxml.jackson.core:jackson-annotations")) |
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 agree that having a single shaded artifact might be better
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.
Thanks @laurit applied the comments
…nstrumentation into openai-instrumentation
OpenAITelemetry.builder(GlobalOpenTelemetry.get()) | ||
.setCaptureMessageContent( | ||
AgentInstrumentationConfig.get() | ||
.getBoolean("otel.instrumentation.genai.capture-message-content", false)) |
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 think we should document this similarly to
| `otel.instrumentation.genai.capture-message-content` | Boolean | `false` | v2 only, record content of user and LLM messages when using Bedrock. | |
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.
Thanks, added a readme
This is initial instrumentation for OpenAI SDK. It is very similar in concept to the previous Bedrock instrumentation.
Future PRs will add support for streaming, async client, embeddings, and responses.
This is adapted from the instrumentation released in elastic's EDOT
/cc @codefromthecrypt