-
Notifications
You must be signed in to change notification settings - Fork 986
Instrument embeddings in openai client #14353
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: main
Are you sure you want to change the base?
Conversation
…-instrumentation into openai-async
…nstrumentation into openai-embeddings
@@ -50,6 +50,7 @@ public interface GenAiAttributesGetter<REQUEST, RESPONSE> { | |||
@Nullable | |||
Double getRequestTopP(REQUEST request); | |||
|
|||
@Nullable |
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.
We initially had discussion that it's confusing if a list can be empty or null, which was definitely correct for chat. But these semantic conventions are shared between chat / embeddings where ones like this one aren't present at all in embeddings, so I guess we should allow null
for it
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.
Since the GenAiAttributesExtractor
treats null
and empty the same way making this nullable isn't strictly necessary. @trask do you have a preference here?
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 - I reverted since indeed the behavior is the same
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.
Pull Request Overview
This PR adds telemetry instrumentation for embeddings operations in the OpenAI Java client. It extends the existing chat completion instrumentation to include embeddings, adding telemetry collection for embedding creation requests and responses.
- Adds instrumentation for OpenAI embeddings API calls
- Creates test infrastructure and test cases for embeddings operations
- Implements attribute extraction for embedding-specific metadata
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
testing/src/main/resources/mappings/* | Test fixture mappings for embedding API responses |
testing/src/main/java/.../Abstract*.java | Base test classes and embedding-specific test implementation |
library/src/test/java/.../EmbeddingsTest.java | Library-specific embeddings test |
library/src/main/java/.../OpenAITelemetry*.java | Updated telemetry builder and main class to support embeddings |
library/src/main/java/.../Instrumented*.java | Instrumented client classes updated to handle embeddings services |
library/src/main/java/.../InstrumentedEmbedding*.java | New embedding service instrumentation classes |
library/src/main/java/.../EmbeddingAttributesGetter.java | Attribute getter implementation for embeddings |
library/src/main/java/.../GenAiAttributes.java | Added embeddings operation name constant |
javaagent/src/test/java/.../EmbeddingsTest.java | JavaAgent-specific embeddings test |
instrumentation-api-incubator/.../GenAiAttributesGetter.java | Added @nullable annotation |
...sting/src/main/java/io/opentelemetry/instrumentation/openai/v1_1/AbstractEmbeddingsTest.java
Show resolved
Hide resolved
...sting/src/main/java/io/opentelemetry/instrumentation/openai/v1_1/AbstractEmbeddingsTest.java
Show resolved
Hide resolved
@@ -15,6 +15,7 @@ final class GenAiAttributes { | |||
|
|||
static final class GenAiOperationNameIncubatingValues { | |||
static final String CHAT = "chat"; | |||
static final String EMBEDDING = "embeddings"; |
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.
maybe a nitpick, but should this be EMBEDDINGS
to match the value? Maybe not, since it's singular in all the other class names
…nstrumentation into openai-embeddings
.addAttributesExtractor( | ||
GenAiAttributesExtractor.create(EmbeddingAttributesGetter.INSTANCE)) | ||
.addOperationMetrics(GenAiClientMetrics.get()) | ||
.buildInstrumenter(SpanKindExtractor.alwaysClient()); |
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 realized I had the wrong span kind. I will fix / assert for chat spans in a separate PR
…nstrumentation into openai-embeddings
No description provided.