GenAI Utils | Adding Embedding metrics #4377
GenAI Utils | Adding Embedding metrics #4377shuningc wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
| self._record_embedding_metrics( | ||
| invocation, span, error_type=error_type | ||
| self._record_metrics(invocation, span, error_type=error_type) | ||
| _maybe_emit_embedding_event( |
There was a problem hiding this comment.
I don't think we have embedding event documented in semantic conventions. The one we have (gen_ai.client.inference.operation.details https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-events.md#event-gen_aiclientinferenceoperationdetails) is for inference.
I believe the only reason we wanted event for it is to capture large content, but we never added any of the content attributes to embedding span - https://github.com/open-telemetry/semantic-conventions/blob/main/docs/gen-ai/gen-ai-spans.md#embeddings
So I think we should not report any events for embeddings
There was a problem hiding this comment.
Removed events emissions for Embedding type
shuningc
changed the title
| attributes: Dict[str, AttributeValue] = {} | ||
|
|
||
| # Set attributes using getattr for fields that may not exist on base class | ||
| operation_name = getattr(invocation, "operation_name", None) |
There was a problem hiding this comment.
is there a case when operation name is not available in the base model? it shouid not be
| if request_model: | ||
| attributes[GenAI.GEN_AI_REQUEST_MODEL] = request_model | ||
|
|
||
| provider = getattr(invocation, "provider", None) |
There was a problem hiding this comment.
is there a case when provider is not available? it should always be available
| if server_port is not None: | ||
| attributes[server_attributes.SERVER_PORT] = server_port | ||
|
|
||
| metric_attributes = getattr(invocation, "metric_attributes", None) |
There was a problem hiding this comment.
should we make metric_attributes part of base operation definition?
There was a problem hiding this comment.
Moved to base operation definition, together with operation name and provider.
| context=span_context, | ||
| ) | ||
| # Only record token metrics for LLMInvocation | ||
| if isinstance(invocation, LLMInvocation): |
There was a problem hiding this comment.
why only LLM? we should still report input token usage for embeddings
There was a problem hiding this comment.
Added input token metrics for embeddings
shuningc
force-pushed
the
addingEmbeddingMetric
branch
from
e6216e4 to
68d328f
Compare
5 participants
Description
This PR adds metrics and events telemetry for [EmbeddingInvocation], aligning with OpenTelemetry semantic conventions. Previously, embedding invocations only emitted spans - now they also emit duration metrics and operation details events.
Type of change
How Has This Been Tested?
Added 3 new tests for embedding metrics:
[test_stop_embedding_records_duration_only] - verifies duration is recorded but token metrics are NOT
[test_stop_embedding_records_duration_with_additional_attributes] - verifies server address, port, custom attributes, and response model are included
[test_fail_embedding_records_error_and_duration] - verifies error path records
error.type and duration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.