feat(semantic-conventions-ai): move custom histogram to semantic-conventions-ai #3181
Conversation
WalkthroughThis change introduces a new package, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstrumentationPkg as Instrumentation Package (_instrument)
participant SemconvAI as opentelemetry-semconv_ai
participant MeterProvider
User->>InstrumentationPkg: Call _instrument(**kwargs)
alt meter_provider not supplied
InstrumentationPkg->>SemconvAI: apply_genai_bucket_configuration()
SemconvAI->>MeterProvider: Set global MeterProvider with GenAI views
else meter_provider supplied
InstrumentationPkg->>MeterProvider: Use provided MeterProvider
end
InstrumentationPkg->>MeterProvider: get_meter()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7be622d in 1 minute and 3 seconds. Click for details.
- Reviewed
1247lines of code in21files - Skipped
4files when reviewing. - Skipped posting
6draft 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/traceloop-sdk/traceloop/sdk/metrics/metrics.py:30
- Draft comment:
Consider adding thread synchronization (e.g. a lock) in new to ensure thread safety for the singleton initialization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py:32
- Draft comment:
If MetricsWrapper.endpoint is not set, the instance is returned without initializing __metrics_exporter and __metrics_provider. Consider clarifying this behavior or raising an exception. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py:87
- Draft comment:
MetricViewsBuilder.get_all_genai_views() is used to set custom views for the MeterProvider. Ensure these views meet your requirements and that the global meter provider is updated as intended. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
4. packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py:601
- Draft comment:
Minor typographical note: consider changing "inited" to "initialized" in the comment for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
5. packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.py:589
- Draft comment:
Typo: The comment uses "inited" which is not standard. Consider changing it to "initialized" for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
6. packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py:417
- Draft comment:
Typographical suggestion: In the comment "meter and counters are inited here", it may be clearer to use "initialized" instead of "inited". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_w2sPuDcSnsq1inom
You can customize 
minimAluminiumalism
force-pushed
the
bucket
branch
from
7be622d to
1d1561f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (2)
85-117: Improve the final assertion.Good integration test for Pinecone metrics, but the final
assert Trueis meaningless. Consider either removing it or adding a more meaningful assertion that verifies the histogram operations completed successfully.- assert True + # Test passes if no exceptions are raised during metric recordingOr remove the assertion entirely if the test's purpose is just to verify no exceptions are thrown.
230-239: Fix Yoda conditions for better style consistency.The test logic is correct, but the equality comparisons use "Yoda conditions" (constant == variable) which goes against Python style conventions.
- assert MetricBuckets.LLM_TOKEN_USAGE == original_llm_buckets - assert MetricBuckets.PINECONE_DB_QUERY_DURATION == original_duration_buckets - assert MetricBuckets.PINECONE_DB_QUERY_SCORES == original_score_buckets + assert original_llm_buckets == MetricBuckets.LLM_TOKEN_USAGE + assert original_duration_buckets == MetricBuckets.PINECONE_DB_QUERY_DURATION + assert original_score_buckets == MetricBuckets.PINECONE_DB_QUERY_SCORES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/opentelemetry-instrumentation-pinecone/poetry.lockis excluded by!**/*.lockpackages/opentelemetry-semantic-conventions-ai/poetry.lockis excluded by!**/*.lockpackages/sample-app/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py(1 hunks)packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.py(1 hunks)packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py(1 hunks)packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(1 hunks)packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py(1 hunks)packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v0/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai/tests/metrics/test_bucket_configuration.py(1 hunks)packages/opentelemetry-instrumentation-openai/tests/metrics/test_bucket_integration.py(1 hunks)packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.py(1 hunks)packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/pyproject.toml(1 hunks)packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/tests/test_metric_buckets.py(1 hunks)packages/sample-app/pyproject.toml(1 hunks)packages/sample-app/sample_app/openai_standalone_instrument.py(1 hunks)packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/opentelemetry-semantic-conventions-ai/pyproject.toml
- packages/sample-app/sample_app/openai_standalone_instrument.py
- packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/init.py
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v0/init.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/init.py
- packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/init.py
- packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/init.py
- packages/sample-app/pyproject.toml
- packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/init.py
- packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/init.py
- packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/init.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/init.py
- packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/init.py
- packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/init.py
- packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py
- packages/opentelemetry-instrumentation-openai/tests/metrics/test_bucket_configuration.py
- packages/opentelemetry-instrumentation-openai/tests/metrics/test_bucket_integration.py
- packages/opentelemetry-semantic-conventions-ai/tests/test_metric_buckets.py
- packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (2)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (7)
MetricBuckets(277-332)MetricViewsBuilder(335-410)Meters(6-33)get_all_genai_views(402-410)get_llm_token_usage_view(339-347)get_pinecone_query_duration_view(350-358)get_pinecone_query_scores_view(361-369)packages/opentelemetry-instrumentation-milvus/tests/conftest.py (1)
meter_provider(45-50)
🪛 Ruff (0.12.2)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py
165-165: Loop control variable i not used within loop body
(B007)
236-236: Yoda condition detected
(SIM300)
237-237: Yoda condition detected
(SIM300)
238-238: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.9)
- GitHub Check: Test Packages (3.12)
🔇 Additional comments (7)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (7)
1-8: LGTM!Clean imports and descriptive module docstring. The imports are well-organized and follow Python conventions.
13-52: LGTM!Good integration test that verifies custom metric views work with MeterProvider. The early return pattern for handling missing OpenTelemetry SDK dependencies is appropriate, and the test thoroughly validates view creation, histogram creation, and metric recording.
53-71: Good test coverage with a minor consideration.The test properly validates individual view creation and instrument name assignment. Note that accessing private attributes like
_instrument_namemay be brittle if the OpenTelemetry SDK changes its internal API, but this appears necessary for thorough testing of the integration.
72-84: LGTM!Effective test that verifies bucket configurations are properly applied to views. The use of private attributes is necessary for this level of integration testing.
118-144: LGTM!Excellent compatibility test that verifies views work across different OpenTelemetry versions. The distinction between default aggregation for operation duration and explicit bucket aggregation for other metrics is correctly handled.
192-216: LGTM!Excellent test coverage for similarity score buckets. The method thoroughly validates bucket boundaries, ordering, and coverage for typical score values including edge cases.
221-229: LGTM!Good defensive test that ensures view creation works gracefully even when OpenTelemetry SDK dependencies might be missing. This helps maintain robustness in different deployment scenarios.
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py
Outdated
Show resolved
Hide resolved
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py
Outdated
Show resolved
Hide resolved
minimAluminiumalism
force-pushed
the
bucket
branch
from
1d1561f to
4d1e090
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (1)
85-117: Replace weak assertion with meaningful verification or remove it.The test properly creates and records values to Pinecone histograms, but ends with
assert Truewhich doesn't verify anything meaningful.Consider one of these alternatives:
- assert True + # Test passes if no exceptions are raised during recordingOr add meaningful assertions:
- assert True + # Verify histograms were created successfully + assert duration_histogram is not None + assert score_histogram is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/opentelemetry-instrumentation-pinecone/poetry.lockis excluded by!**/*.lockpackages/opentelemetry-semantic-conventions-ai/poetry.lockis excluded by!**/*.lockpackages/sample-app/poetry.lockis excluded by!**/*.lockpackages/traceloop-sdk/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py(1 hunks)packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/__init__.py(1 hunks)packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py(1 hunks)packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(1 hunks)packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/__init__.py(1 hunks)packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v0/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/__init__.py(1 hunks)packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/__init__.py(1 hunks)packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/pyproject.toml(1 hunks)packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/tests/test_metric_buckets.py(1 hunks)packages/sample-app/pyproject.toml(1 hunks)packages/sample-app/sample_app/openai_standalone_instrument.py(1 hunks)packages/traceloop-sdk/pyproject.toml(1 hunks)packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/opentelemetry-semantic-conventions-ai/pyproject.toml
- packages/traceloop-sdk/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/init.py
- packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/init.py
- packages/opentelemetry-instrumentation-milvus/opentelemetry/instrumentation/milvus/init.py
- packages/opentelemetry-instrumentation-pinecone/opentelemetry/instrumentation/pinecone/init.py
- packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py
- packages/opentelemetry-instrumentation-bedrock/opentelemetry/instrumentation/bedrock/init.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v1/init.py
- packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/init.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/v0/init.py
- packages/traceloop-sdk/traceloop/sdk/metrics/metrics.py
- packages/sample-app/pyproject.toml
- packages/opentelemetry-instrumentation-ollama/opentelemetry/instrumentation/ollama/init.py
- packages/sample-app/sample_app/openai_standalone_instrument.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/init.py
- packages/opentelemetry-instrumentation-watsonx/opentelemetry/instrumentation/watsonx/init.py
- packages/opentelemetry-semantic-conventions-ai/tests/test_metric_buckets.py
- packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (1)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (7)
MetricBuckets(277-332)MetricViewsBuilder(335-410)Meters(6-33)get_all_genai_views(402-410)get_llm_token_usage_view(339-347)get_pinecone_query_duration_view(350-358)get_pinecone_query_scores_view(361-369)
🪛 Ruff (0.12.2)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py
236-236: Yoda condition detected
(SIM300)
237-237: Yoda condition detected
(SIM300)
238-238: Yoda condition detected
(SIM300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.9)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Lint
- GitHub Check: Build Packages (3.11)
🔇 Additional comments (11)
packages/opentelemetry-semantic-conventions-ai/tests/test_integration.py (11)
1-8: LGTM! Clean and appropriate imports.The imports are well-organized and import exactly what's needed for the integration tests.
10-11: LGTM! Clear class purpose and documentation.The class name and docstring clearly indicate this is for integration testing between MetricViewsBuilder and OpenTelemetry SDK.
13-52: LGTM! Comprehensive integration test with proper error handling.The test properly:
- Handles optional OpenTelemetry SDK dependencies with try/except
- Verifies view count and types
- Creates and tests histograms with actual data recording
- Uses meaningful assertions
53-71: LGTM! Individual view creation test is well-structured.The test properly verifies:
- Individual view creation methods
- View type assertions
- Instrument name verification using private attributes (acceptable in tests)
72-84: LGTM! Proper bucket configuration verification.The test correctly verifies:
- Aggregation type (ExplicitBucketHistogramAggregation)
- Bucket boundaries match the expected values
- Appropriate use of private attributes for testing
118-144: LGTM! Comprehensive compatibility testing.The test properly:
- Verifies view attributes across different versions
- Handles different aggregation types appropriately
- Tests both default and explicit bucket aggregations
- Includes actual histogram creation and recording
149-170: LGTM! Past review comments have been addressed.The method name now correctly reflects that it tests
LLM_TOKEN_USAGEbucket distribution, and the unused loop variable issue has been resolved. The test logic is sound and comprehensive.
171-191: LGTM! Method name correctly reflects the test purpose.The test properly verifies Pinecone query duration bucket distribution with appropriate assertions for bucket sorting, boundaries, and coverage.
192-216: LGTM! Comprehensive similarity score bucket testing.The test thoroughly verifies:
- Bucket count and key boundary values
- Proper sorting order
- Complete range coverage (-1 to 1)
- Representative score value coverage
221-229: LGTM! Good error handling verification.The test ensures that
MetricViewsBuildercan create views even when OpenTelemetry SDK dependencies are missing, which demonstrates good robustness in the implementation.
230-239: LGTM! Proper immutability testing with false positive static analysis warnings.The test correctly verifies that bucket definitions remain unchanged during test execution. The static analysis warnings about "Yoda conditions" are false positives - these are normal equality comparisons between class attributes and copied values.
minimAluminiumalism
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (2)
366-375: Import placement could be optimized.The import statements are placed inside the method, which is repeated across multiple methods. Consider moving these imports to the module level or using a shared import pattern for better performance.
Move imports to module level:
+from opentelemetry.sdk.metrics.view import View, ExplicitBucketHistogramAggregation + class MetricViewsBuilder: """Builder for OpenTelemetry metric views with predefined buckets.""" @staticmethod def get_llm_token_usage_view(): - from opentelemetry.sdk.metrics.view import View, ExplicitBucketHistogramAggregation - return View(
441-491: Consider adding configuration options and validation.The function hardcodes the GenAI views without allowing customization. Consider adding parameters to enable/disable specific metrics or allow custom bucket configurations for different use cases.
Add configuration options:
-def apply_genai_bucket_configuration(): +def apply_genai_bucket_configuration(*, + enabled_metrics=None, + custom_buckets=None, + force_reapply=False): """Apply GenAI bucket configuration to the global MeterProvider. Args: + enabled_metrics: List of metric names to configure (default: all) + custom_buckets: Dict of metric_name -> bucket_boundaries overrides + force_reapply: Whether to reapply even if already configured """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/sample-app/poetry.lockis excluded by!**/*.lockpackages/traceloop-sdk/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/__init__.py(1 hunks)packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py(1 hunks)packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/__init__.py(1 hunks)packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py(1 hunks)packages/opentelemetry-semantic-conventions-ai/pyproject.toml(1 hunks)packages/traceloop-sdk/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/opentelemetry-semantic-conventions-ai/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/traceloop-sdk/pyproject.toml
- packages/opentelemetry-instrumentation-anthropic/opentelemetry/instrumentation/anthropic/init.py
- packages/opentelemetry-instrumentation-openai-agents/opentelemetry/instrumentation/openai_agents/init.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/init.py
- packages/opentelemetry-instrumentation-langchain/opentelemetry/instrumentation/langchain/init.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Packages (3.12)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Lint
🔇 Additional comments (3)
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (3)
305-323: Well-designed exponential bucket boundaries for token usage.The
LLM_TOKEN_USAGEbuckets use a power-of-4 progression (1, 4, 16, 64...) which provides good coverage for typical LLM token usage patterns, from small prompts to very large contexts up to 67M tokens.
342-360: Score bucket boundaries cover full similarity range correctly.The score buckets appropriately cover the typical similarity score range [-1, 1] with reasonable granularity (0.125 increments), suitable for cosine similarity and other normalized distance metrics.
325-340: Double-check Pinecone query duration bucketsI ran a search for any existing Pinecone timeout or latency settings (
rg -A 3 -B 3 "timeout.*pinecone|pinecone.*timeout" --type py) and found no references in the codebase. Please verify that the upper duration buckets (20.48 s, 40.96 s, 81.92 s) actually reflect expected Pinecone query latencies rather than timeouts or underlying system issues.
| def get_llm_operation_duration_view(): | ||
| from opentelemetry.sdk.metrics.view import View | ||
|
|
||
| return View( | ||
| instrument_name=Meters.LLM_OPERATION_DURATION, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LLM operation duration view lacks explicit aggregation configuration.
Unlike other views, this method doesn't specify an ExplicitBucketHistogramAggregation, which means it will use the default histogram buckets. This inconsistency might lead to suboptimal bucket boundaries for duration metrics.
Add explicit bucket configuration for consistency:
@staticmethod
def get_llm_operation_duration_view():
- from opentelemetry.sdk.metrics.view import View
+ from opentelemetry.sdk.metrics.view import View, ExplicitBucketHistogramAggregation
return View(
instrument_name=Meters.LLM_OPERATION_DURATION,
+ aggregation=ExplicitBucketHistogramAggregation(
+ MetricBuckets.PINECONE_DB_QUERY_DURATION # or define LLM_OPERATION_DURATION buckets
+ ),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_llm_operation_duration_view(): | |
| from opentelemetry.sdk.metrics.view import View | |
| return View( | |
| instrument_name=Meters.LLM_OPERATION_DURATION, | |
| ) | |
| @staticmethod | |
| def get_llm_operation_duration_view(): | |
| - from opentelemetry.sdk.metrics.view import View | |
| + from opentelemetry.sdk.metrics.view import View, ExplicitBucketHistogramAggregation | |
| - return View( | |
| - instrument_name=Meters.LLM_OPERATION_DURATION, | |
| - ) | |
| + return View( | |
| + instrument_name=Meters.LLM_OPERATION_DURATION, | |
| + aggregation=ExplicitBucketHistogramAggregation( | |
| + MetricBuckets.PINECONE_DB_QUERY_DURATION # or define LLM_OPERATION_DURATION buckets | |
| + ), | |
| + ) |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 400 to 405, the get_llm_operation_duration_view function returns a
View without specifying an ExplicitBucketHistogramAggregation, causing it to use
default histogram buckets. To fix this, import
ExplicitBucketHistogramAggregation and add an explicit aggregation parameter
with appropriate bucket boundaries to the View constructor, ensuring consistent
and optimal histogram bucket configuration for duration metrics.
| aggregation=ExplicitBucketHistogramAggregation( | ||
| MetricBuckets.PINECONE_DB_QUERY_DURATION | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reusing Pinecone duration buckets for generic DB queries may not be appropriate.
The DB_QUERY_DURATION view uses PINECONE_DB_QUERY_DURATION buckets, but different database systems may have vastly different performance characteristics. Consider defining separate buckets or using more generic duration buckets.
Consider adding generic database duration buckets:
class MetricBuckets:
# ... existing buckets ...
+
+ DB_QUERY_DURATION = [
+ 0.001, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5,
+ 0.75, 1.0, 2.5, 5.0, 7.5, 10.0, 30.0, 60.0
+ ]Then update the view:
aggregation=ExplicitBucketHistogramAggregation(
- MetricBuckets.PINECONE_DB_QUERY_DURATION
+ MetricBuckets.DB_QUERY_DURATION
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aggregation=ExplicitBucketHistogramAggregation( | |
| MetricBuckets.PINECONE_DB_QUERY_DURATION | |
| ), | |
| ) | |
| # in packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py | |
| class MetricBuckets: | |
| # ... existing buckets ... | |
| # generic database query duration buckets | |
| DB_QUERY_DURATION = [ | |
| 0.001, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, | |
| 0.75, 1.0, 2.5, 5.0, 7.5, 10.0, 30.0, 60.0 | |
| ] |
| aggregation=ExplicitBucketHistogramAggregation( | |
| MetricBuckets.PINECONE_DB_QUERY_DURATION | |
| ), | |
| ) | |
| # later in the same file, update the view definition: | |
| aggregation=ExplicitBucketHistogramAggregation( | |
| MetricBuckets.DB_QUERY_DURATION | |
| ), |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 413 to 416, the DB_QUERY_DURATION view is using
PINECONE_DB_QUERY_DURATION buckets which are specific to Pinecone and may not
suit other databases. Define a new set of generic database duration buckets that
better represent typical DB query durations across various systems, then update
the aggregation in the view to use these new generic buckets instead of the
Pinecone-specific ones.
| aggregation=ExplicitBucketHistogramAggregation( | ||
| MetricBuckets.PINECONE_DB_QUERY_SCORES | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reusing score buckets for distance metrics may be semantically incorrect.
Distance metrics often have different ranges and semantics than similarity scores. For example, Euclidean distance is typically non-negative and unbounded, while cosine similarity is bounded [-1, 1].
Consider defining separate buckets for distance metrics:
class MetricBuckets:
# ... existing buckets ...
+
+ DB_SEARCH_DISTANCE = [
+ 0, 0.1, 0.25, 0.5, 0.75, 1.0, 1.5, 2.0, 3.0, 5.0, 10.0, 25.0, 50.0, 100.0
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aggregation=ExplicitBucketHistogramAggregation( | |
| MetricBuckets.PINECONE_DB_QUERY_SCORES | |
| ), | |
| ) | |
| class MetricBuckets: | |
| # ... existing buckets ... | |
| DB_SEARCH_DISTANCE = [ | |
| 0, 0.1, 0.25, 0.5, 0.75, 1.0, 1.5, 2.0, 3.0, 5.0, 10.0, 25.0, 50.0, 100.0 | |
| ] |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 424 to 427, the code reuses score buckets designed for similarity
scores in distance metric aggregations, which is semantically incorrect due to
differing value ranges. Define and use a separate set of metric buckets
specifically tailored for distance metrics, ensuring the bucket ranges align
with the expected value domain of the distance metric being measured.
| if hasattr(current_provider, '_genai_buckets_applied'): | ||
| return |
There was a problem hiding this comment.
Thread safety concern with provider modification.
The flag _genai_buckets_applied is checked and set without synchronization, which could lead to race conditions in multi-threaded applications where multiple threads might simultaneously call this function.
Add thread synchronization:
+import threading
+
+_config_lock = threading.Lock()
+
def apply_genai_bucket_configuration():
"""Apply GenAI bucket configuration to the global MeterProvider."""
+ with _config_lock:
try:
# ... rest of the function📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if hasattr(current_provider, '_genai_buckets_applied'): | |
| return | |
| import threading | |
| _config_lock = threading.Lock() | |
| def apply_genai_bucket_configuration(): | |
| """Apply GenAI bucket configuration to the global MeterProvider.""" | |
| with _config_lock: | |
| try: | |
| if hasattr(current_provider, '_genai_buckets_applied'): | |
| return | |
| # ... rest of the function |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 449 to 450, the check and setting of the _genai_buckets_applied
flag on current_provider is not thread-safe, risking race conditions. To fix
this, introduce a threading lock to synchronize access to this flag, ensuring
that only one thread can check and set it at a time. Use a global or
module-level lock object and acquire it before checking or setting the flag,
then release it afterward.
| if ('NoOp' in provider_type or 'Proxy' in provider_type or | ||
| not hasattr(current_provider, '__module__') or | ||
| current_provider.__class__.__name__ == 'ProxyMeterProvider'): | ||
| new_provider = MeterProvider(views=genai_views) | ||
| new_provider._genai_buckets_applied = True | ||
| metrics.set_meter_provider(new_provider) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provider detection logic may be fragile.
The string-based detection using 'NoOp' in provider_type and 'Proxy' in provider_type is brittle and could break with OpenTelemetry SDK updates or custom provider implementations.
Use more robust type checking:
- if ('NoOp' in provider_type or 'Proxy' in provider_type or
- not hasattr(current_provider, '__module__') or
- current_provider.__class__.__name__ == 'ProxyMeterProvider'):
+ from opentelemetry.metrics import NoOpMeterProvider
+ from opentelemetry.metrics._internal import ProxyMeterProvider
+
+ if (isinstance(current_provider, (NoOpMeterProvider, ProxyMeterProvider)) or
+ not hasattr(current_provider, '_all_metric_readers')):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ('NoOp' in provider_type or 'Proxy' in provider_type or | |
| not hasattr(current_provider, '__module__') or | |
| current_provider.__class__.__name__ == 'ProxyMeterProvider'): | |
| new_provider = MeterProvider(views=genai_views) | |
| new_provider._genai_buckets_applied = True | |
| metrics.set_meter_provider(new_provider) | |
| from opentelemetry.metrics import NoOpMeterProvider | |
| from opentelemetry.metrics._internal import ProxyMeterProvider | |
| if (isinstance(current_provider, (NoOpMeterProvider, ProxyMeterProvider)) or | |
| not hasattr(current_provider, '_all_metric_readers')): | |
| new_provider = MeterProvider(views=genai_views) | |
| new_provider._genai_buckets_applied = True | |
| metrics.set_meter_provider(new_provider) |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 455 to 460, the current provider detection uses fragile string
checks like 'NoOp' in provider_type and 'Proxy' in provider_type. Replace these
with robust type checking by importing and using the actual NoOpMeterProvider
and ProxyMeterProvider classes from the OpenTelemetry SDK and checking if
current_provider is an instance of these classes using isinstance(). This will
make the detection more reliable against SDK changes or custom implementations.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Silent exception handling may hide important errors.
The bare except Exception: pass blocks suppress all exceptions, which could hide configuration errors, import issues, or other problems that should be logged or handled appropriately.
Add proper error handling and logging:
+import logging
+
+logger = logging.getLogger(__name__)
+
except Exception as e:
- pass
+ logger.debug(f"Failed to recreate metric reader: {e}")
+ continueApply similar changes to other exception blocks.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
around lines 477 to 478, the bare except Exception: pass block silently
suppresses all exceptions, potentially hiding important errors. Modify this
block to catch the exception, log the error details using an appropriate logger
or print statement, and handle the error properly instead of passing silently.
Apply similar error handling and logging improvements to other bare except
blocks in the file.
| import opentelemetry.metrics._internal as metrics_internal | ||
| metrics_internal._METER_PROVIDER = new_provider |
There was a problem hiding this comment.
Direct manipulation of internal OpenTelemetry state is risky.
Directly setting metrics_internal._METER_PROVIDER bypasses the official API and could break with future OpenTelemetry versions or cause unexpected behavior.
Use the official API instead:
- import opentelemetry.metrics._internal as metrics_internal
- metrics_internal._METER_PROVIDER = new_provider
+ metrics.set_meter_provider(new_provider)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import opentelemetry.metrics._internal as metrics_internal | |
| metrics_internal._METER_PROVIDER = new_provider | |
| @@ -486,2 +486,1 @@ | |
| - import opentelemetry.metrics._internal as metrics_internal | |
| - metrics_internal._METER_PROVIDER = new_provider | |
| + metrics.set_meter_provider(new_provider) |
🤖 Prompt for AI Agents
In
packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py
at lines 486-487, avoid directly setting the internal variable
metrics_internal._METER_PROVIDER as it bypasses the official API and risks
future compatibility. Instead, replace this direct assignment with calls to the
official OpenTelemetry API for setting or updating the meter provider, ensuring
you use the supported methods to configure the meter provider safely.
feat(instrumentation): ...orfix(instrumentation): ....Feat #3146
Note: Since I've changed
opentelemetry-semantic-conventions-aiwhich is a dependency of other instrumentation packages, more unit tests of metric foropenetelemetry-instrumentation-xxxwill be added in a follow-up PR after a new verison of opentelemetry-semantic-conventions-ai released(maybe v0.4.13) to avoid local path dependencies in pyproject.toml. https://github.com/traceloop/openllmetry/blob/cfe309d0658b9a9fd9ffd6045a0e24a3b2be3f7c/packages/opentelemetry-instrumentation-openai/pyproject.toml#L30C1-L30C38Currently I've only change the dependency of
opentelemetry-semantic-conventions-aito local dev undertraceloop-sdkto pass the tests. We may need to revert it after the new semconv is released.https://github.com/traceloop/openllmetry/pull/3181/files#diff-f576c311dca29ec53bc94ff0163289ed5bea0734e5614784440df1972cf8ee24R38
I've also add a example under
sample_appto verified this change is effective locally.Important
Centralizes custom histogram configurations in
opentelemetry-semantic-conventions-aiand applies them across various instrumentations, with added tests for verification.opentelemetry-semantic-conventions-ai.apply_genai_bucket_configuration()in_instrument()methods across multiple instrumentors (e.g.,AnthropicInstrumentor,BedrockInstrumentor,CrewAIInstrumentor).test_bucket_configuration.pyandtest_bucket_integration.py.MetricBucketsandMetricViewsBuilderinsemconv_ai/__init__.py.LLM_TOKEN_USAGEandPINECONE_DB_QUERY_DURATION.test_integration.pyandtest_metric_buckets.pyto verify bucket configurations and view creation.pyproject.tomlto include new dependencies for testing.openai_standalone_instrument.pyto demonstrate metric setup.This description was created by
for 7be622d. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores