-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Support OpenAI embeddings API #18224
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
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
size-limit report 📦
|
RulaKhaled
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.
Nice! Just a quick comment and then we're good to go
| /** | ||
| * Add attributes for Embeddings API responses | ||
| */ | ||
| function addEmbeddingsAttributes(span: Span, response: OpenAICreateEmbeddingsObject): void { |
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.
can we move this to utils? if max number of lines complain, we can keep this 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.
yes, just put it there because similar methods for completions and responses were in the index file. moved them all to utils
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.
amazing! i'm trying to clean this up more moving forward
| span.setAttributes({ [GEN_AI_RESPONSE_TEXT_ATTRIBUTE]: response.output_text }); | ||
| } | ||
| } else if (isEmbeddingsResponse(response)) { | ||
| addEmbeddingsAttributes(span, response); |
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.
Bug: Array Truncation Causes Embeddings Data Loss
When embeddings input is an array of strings requiring truncation, getTruncatedJsonString incorrectly processes it through truncateGenAiMessages, which expects message objects with content or parts properties. For plain strings, truncation fails and returns an empty array, causing data loss. The embeddings API accepts both single strings and arrays of strings, but only single string truncation works correctly.
RulaKhaled
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.
Thanks!
This adds instrumentation for the OpenAI Embeddings API. Specifically, we instrument Create embeddings, which is also the only endpoint in the embeddings API atm. Implementation generally follows the same flow we also have for the
completionsandresponsesAPIs. To detectembeddingrequests we check whether the model name containsembeddings.The embedding results are currently not tracked, as we do not truncate outputs right now as far as I know and these can get large quite easily. For instance, text-embedding-3 uses dimension 1536 (small) or 3072 (large) by default, resulting in single embeddings sizes of 6KB and 12KB, respectively.
Test updates: