feat(openai): add gen_ai.request.model to meter attributes and update doc, ... #3370
Conversation
WalkthroughAdds gen_ai.request.model as a meter attribute in streaming chat metrics, documents the new meter attributes in README, and updates tests to assert the attribute presence and value. No public APIs or control flow changed. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to d2607d7 in 1 minute and 50 seconds. Click for details.
- Reviewed
74lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/opentelemetry-instrumentation-openai/README.md:10
- Draft comment:
Good documentation update! The description of the new meter attribute is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py:50
- Draft comment:
Test assertions check that 'gen_ai.request.model' equals the expected model. Ensure the non-streaming instrumentation is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the non-streaming instrumentation is updated accordingly. This is a vague request for confirmation or assurance, which violates the rule against asking the author to ensure something is tested or intended. It does not provide a specific suggestion or point out a specific issue.
Workflow ID: wflow_CTX4TqkqWp4J4vOr
You can customize 
|
|
||
| shared_attributes = { | ||
| SpanAttributes.LLM_RESPONSE_MODEL: complete_response.get("model") or None, | ||
| "gen_ai.request.model": request_kwargs.get("model") if request_kwargs else None, |
There was a problem hiding this comment.
New attribute 'gen_ai.request.model' is added for streaming responses. However, non-streaming completions (handled via _handle_response/_set_chat_metrics) do not include this attribute. Please update the synchronous path so that all meter data points include 'gen_ai.request.model' to ensure consistency with tests.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (2)
726-734: ChatStream path misses explicit gen_ai.request.model and risks AttributeError when request_kwargs is None
- _shared_attributes() doesn’t attach gen_ai.request.model, so ChatStream (OpenAI v1 streaming) likely omits the new attribute.
- Accessing self._request_kwargs.get(...) without a None/type guard can raise at runtime.
Apply:
def _shared_attributes(self): - return metric_shared_attributes( - response_model=self._complete_response.get("model") - or self._request_kwargs.get("model") - or None, - operation="chat", - server_address=_get_openai_base_url(self._instance), - is_streaming=True, - ) + resp_model = self._complete_response.get("model") + req_model = ( + self._request_kwargs.get("model") + if isinstance(self._request_kwargs, dict) + else None + ) + attrs = metric_shared_attributes( + response_model=resp_model or req_model or None, + operation="chat", + server_address=_get_openai_base_url(self._instance), + is_streaming=True, + ) + # Ensure request model is always stamped on metrics + attrs["gen_ai.request.model"] = req_model + return attrs
1-1192: Addgen_ai.request.modeltometric_shared_attributes
Non-streaming metrics (viametric_shared_attributesinshared/__init__.py) currently only emit system, response model, and operation name—so both regular and ChatStream-based v1 streaming metrics lack the request model. Mirror the manual"gen_ai.request.model": request_kwargs.get("model")added in_build_from_streaming_response(chat_wrappers.py lines 889–894) by updatingmetric_shared_attributesto include the request model.
🧹 Nitpick comments (5)
packages/opentelemetry-instrumentation-openai/README.md (1)
10-18: Clarify release version and nullability; tighten wording
- Replace “vNEXT” with the concrete version at release time.
- Note that gen_ai.request.model may be null when a request model isn’t provided.
- Minor tighten: “This provides richer context...” → “These attributes provide richer context...”.
Apply:
-## Meter Attributes - -As of vNEXT, all meter data points now include both the response and request model names: +## Meter Attributes + +As of vX.Y.Z, all meter data points include both the response and request model names: @@ -* `gen_ai.request.model` — The model name specified in the request payload (e.g., "gpt-3.5-turbo"). +* `gen_ai.request.model` — The model name specified in the request payload (e.g., "gpt-3.5-turbo"). May be null if unspecified. @@ -This provides richer context for metrics and observability. +These attributes provide richer context for metrics and observability.packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (2)
889-896: Good addition; consider centralizing attribute building for consistencyThe direct dict works; however, using metric_shared_attributes(...) and then overlaying gen_ai.request.model keeps attributes uniform (system, operation, etc.) with non-streaming paths.
Apply:
- shared_attributes = { - SpanAttributes.LLM_RESPONSE_MODEL: complete_response.get("model") or None, - "gen_ai.request.model": request_kwargs.get("model") if request_kwargs else None, - "server.address": _get_openai_base_url(instance), - "stream": True, - } + shared_attributes = metric_shared_attributes( + response_model=complete_response.get("model") or None, + operation="chat", + server_address=_get_openai_base_url(instance), + is_streaming=True, + ) + shared_attributes["gen_ai.request.model"] = ( + request_kwargs.get("model") if isinstance(request_kwargs, dict) else None + )
961-967: Mirror the same centralization in async builderKeep async and sync streaming builders aligned to avoid drift.
Apply:
- shared_attributes = { - SpanAttributes.LLM_RESPONSE_MODEL: complete_response.get("model") or None, - "gen_ai.request.model": request_kwargs.get("model") if request_kwargs else None, - "server.address": _get_openai_base_url(instance), - "stream": True, - } + shared_attributes = metric_shared_attributes( + response_model=complete_response.get("model") or None, + operation="chat", + server_address=_get_openai_base_url(instance), + is_streaming=True, + ) + shared_attributes["gen_ai.request.model"] = ( + request_kwargs.get("model") if isinstance(request_kwargs, dict) else None + )packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py (2)
139-226: Add streaming assertions for gen_ai.request.model to cover the streaming metric pathExtend test_chat_streaming_metrics to assert the new attribute for stream=True as well (covers the non-v1 streaming builders; after the ChatStream fix it’ll exercise v1 too when applicable).
Apply:
@@ def test_chat_streaming_metrics(instrument_legacy, reader, deepseek_client): - for metric in sm.metrics: + for metric in sm.metrics: if metric.name == Meters.LLM_TOKEN_USAGE: found_token_metric = True for data_point in metric.data.data_points: assert data_point.attributes[SpanAttributes.LLM_TOKEN_TYPE] in [ "output", "input", ] assert data_point.sum > 0 + # New: request model is stamped on streaming metrics + assert data_point.attributes["gen_ai.request.model"] == "deepseek-chat" @@ if metric.name == Meters.LLM_GENERATION_CHOICES: found_choice_metric = True for data_point in metric.data.data_points: assert data_point.value >= 1 + assert data_point.attributes["gen_ai.request.model"] == "deepseek-chat" @@ if metric.name == Meters.LLM_OPERATION_DURATION: found_duration_metric = True assert any( data_point.count > 0 for data_point in metric.data.data_points ) assert any( data_point.sum > 0 for data_point in metric.data.data_points ) + assert all( + data_point.attributes["gen_ai.request.model"] == "deepseek-chat" + for data_point in metric.data.data_points + )
1-314: Define module‐local constant for request model attribute
The opentelemetry-semantic-conventions-ai package doesn’t expose a SpanAttributes constant for"gen_ai.request.model". Declare a constant in this test module (e.g.REQUEST_MODEL_ATTR = "gen_ai.request.model") and use it instead of the raw string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/opentelemetry-instrumentation-openai/README.md(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py(2 hunks)packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Store API keys only in environment variables/secure vaults; never hardcode secrets in code
Use Flake8 for code linting and adhere to its rules
Files:
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.pypackages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py
🧬 Code graph analysis (1)
packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
Meters(36-61)
🪛 Ruff (0.12.2)
packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py
51-51: Use of assert detected
(S101)
56-56: Use of assert detected
(S101)
57-57: Use of assert detected
(S101)
58-58: Use of assert detected
(S101)
72-72: Use of assert detected
(S101)
🔇 Additional comments (3)
packages/opentelemetry-instrumentation-openai/tests/metrics/test_openai_metrics.py (3)
50-52: LGTM: asserts request model on token usage data points
58-58: LGTM: asserts request model on choices data points
72-75: LGTM: asserts request model on duration data points
Sure, @nirga |
|
|
|
Having some trouble with the failing CI: Kindly requesting some assistance. |
Related issue
Fixes #3144
feat(instrumentation): ...orfix(instrumentation): ....Summary
This PR adds the
gen_ai.request.modelattribute to all meter data points in theOpenAIinstrumentation package, alongside the existinggen_ai.response.model. This provides richer context for metrics and observability.Details
gen_ai.request.modelto meter attributes in both sync and async streaming metric paths.READMEto describe the new attribute.Screenshots
Motivation
Including both the request and response model names in metrics allows for better debugging, analysis, and correlation of model usage, especially when the requested and returned models may differ.
Important
Adds
gen_ai.request.modelto meter attributes in OpenAI instrumentation for enhanced observability, updates tests and documentation.gen_ai.request.modelattribute to meter data points inchat_wrappers.pyfor both sync and async streaming paths.test_openai_metrics.pyto verify the presence and correctness ofgen_ai.request.model.README.mdto includegen_ai.request.modelin the meter attributes section.This description was created by
for d2607d7. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Tests