refactor(openai): migrate embeddings to util-genai EmbeddingInvocation lifecycle#127
Open
JacksonWeber wants to merge 10 commits into
Open
Conversation
…n lifecycle Replaces the legacy embeddings span/metrics plumbing in the OpenAI instrumentation with the shared util-genai EmbeddingInvocation lifecycle, mirroring the chat and responses paths. Drops the now-unused Instruments class, legacy get_llm_request_attributes helper, and embeddings-specific tracer/duration recording. EmbeddingInvocation does not currently include gen_ai.embeddings.dimension.count on metric attributes, so it is set via metric_attributes to preserve semconv conformance on the duration / token.usage metrics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: Claude Opus 4.7 (1M context)
- Rename changelog fragment from next.changed to 127.changed so the changelog check picks it up as <PR>.<type>. - Drop a now-unnecessary '# type: ignore[misc]' comment in the langchain callback handler that the pinned pyright in CI is rejecting (per AGENTS.md, type: ignore comments should not be carried). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: Claude Opus 4.7 (1M context)
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR migrates the OpenAI embeddings instrumentation from legacy, custom span/metrics plumbing to the shared util-genai EmbeddingInvocation lifecycle.
Changes:
- Replaced embeddings span/metrics creation code with
TelemetryHandler.embedding(...)+EmbeddingInvocation.stop()/fail(). - Added
create_embedding_invocation(...)helper to build embeddings invocations with server/model/dimension/encoding metadata. - Removed legacy request-attribute utilities, legacy
Instrumentshelper, and associated unit tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation/opentelemetry-instrumentation-genai-openai/tests/test_request_attributes.py | Removes unit tests that covered request attributes/span naming behavior. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/utils.py | Drops legacy helpers and introduces create_embedding_invocation for the new lifecycle. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/patch.py | Migrates embeddings wrappers to use EmbeddingInvocation and updates response property extraction. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/instruments.py | Deletes legacy metrics helper class used by the previous embeddings path. |
| instrumentation/opentelemetry-instrumentation-genai-openai/src/opentelemetry/instrumentation/genai/openai/init.py | Removes legacy tracer/meter/instruments wiring; passes TelemetryHandler into wrappers. |
| instrumentation/opentelemetry-instrumentation-genai-openai/.changelog/127.changed | Adds changelog entry describing the migration. |
| instrumentation/opentelemetry-instrumentation-genai-langchain/src/opentelemetry/instrumentation/genai/langchain/callback_handler.py | Removes a now-unneeded type: ignore on message.content. |
lmolkova
approved these changes
Jun 12, 2026
- utils.create_embedding_invocation: pass request_model through get_value() so a missing 'model' kwarg stays unset (no empty-string attribute, no trailing space in the span name). - patch.embeddings_create / async_embeddings_create: separate the wrapped OpenAI call from response-property extraction. Extraction errors are now caught and logged via the new _safe_set_embeddings_response_properties helper so unexpected SDK shapes can't break user code. Failures from the wrapped call itself are wrapped in Error(type=..., message=...) for consistency with the chat path. - patch._set_embeddings_response_properties: when dimension_count is inferred from the response, also propagate it to invocation.metric_attributes so the operation duration / token usage metrics carry it (matches the request-side handling). - Add tests/test_embedding_invocation_unit.py covering model omission/setting, span naming, server attrs, dimension-count propagation (request and response paths), encoding_format mapping, swallowed extraction errors, and failed-wrapped-call recording. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Assisted-by: Claude Opus 4.7
…y-python-genai into openai-embeddings-util-genai-migration
Assisted-by: Claude Opus 4.7 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rads-1996
approved these changes
Jun 12, 2026
lmolkova
approved these changes
Jun 12, 2026
eternalcuriouslearner
approved these changes
Jun 14, 2026
Contributor
|
Thanks for your contribution, this looks great!! Only question I have is around |
Move create_embedding_invocation from utils.py into a private _create_embedding_invocation helper in patch.py, shared by the sync and async embeddings wrappers. Also tighten the embeddings response helper signatures to use CreateEmbeddingResponse instead of Any.
Author
|
Tests should be fixed once #133 is merged. |
Contributor
|
Ty!! Everything looks good. Please feel free to resolve the comments, I couldn't resolve them. |
lzchen
approved these changes
Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements PR2 from #107 — migrates the OpenAI embeddings instrumentation to the shared
util-genaiEmbeddingInvocationlifecycle, mirroring the chat and responses paths already on the new model.Changes
patch.py—embeddings_create/async_embeddings_createnow useTelemetryHandler.embedding()with thestop()/fail()lifecycle. Removed the legacy_get_embeddings_span_name,_record_metrics,_set_embeddings_response_attributeshelpers and the manual tracer / duration plumbing.utils.py— Addedcreate_embedding_invocation. Removedget_llm_request_attributes,set_span_attribute(s), andhandle_span_exception(all dead after the migration).__init__.py— Embeddings wrappers now take just the sharedhandler; droppedInstruments,get_tracer,get_meter,Schemasimports.instruments.py— Deleted (util-genai owns embeddings metrics now).tests/test_request_attributes.py— Deleted (only exercised the removed legacy helpers).Gaps / concerns
EmbeddingInvocationinutil-genaidoes not currently putgen_ai.embeddings.dimension.counton metric attributes (only the span). To preserve semconv conformance on thegen_ai.client.operation.duration/gen_ai.client.token.usagemetrics,create_embedding_invocationwrites it intoinvocation.metric_attributeswith a comment. Worth a follow-up in util-genai itself.Validation
tox -e py312-test-instrumentation-genai-openai-latest: 145 passed, 7 failed. All 7 failures are the pre-existingAPIConnectionErrorvsAPITimeoutErrorflake on*_bad_endpoint/*_connection_errortests across chat, embeddings, and responses — verified failing onmainwith the same diff.pre-commit run ruff: clean.