feat(instrumentation): remove param enrich_token_usage and simplify token calculation
#3205
Conversation
… token calculation
|
Warning Rate limit exceeded@minimAluminiumalism has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis change removes the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Instrumentor
participant API
User->>Instrumentor: Initialize (no enrich_token_usage)
User->>Instrumentor: Send request to API
Instrumentor->>API: Make API call
API-->>Instrumentor: Respond with usage data (if available)
Instrumentor->>Instrumentor: Extract token usage from API response (no local counting)
Instrumentor-->>User: Return response and metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. 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. 🪧 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 (
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to d77d742 in 1 minute and 39 seconds. Click for details.
- Reviewed
1781lines of code in18files - Skipped
1files when reviewing. - Skipped posting
5draft 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-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.py:172
- Draft comment:
Removal of the unused parameter is clean; the init now only accepts exception_logger and use_legacy_attributes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it simply states what the code change does without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements, nor does it point out any potential issues. Therefore, it should be removed according to the rules.
2. packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/config.py:1
- Draft comment:
Config now only contains exception_logger and use_legacy_attributes. Check that any external documentation is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that external documentation is updated, which falls under the rule of not asking the author to update documentation or descriptions. It doesn't provide a specific code suggestion or highlight a potential issue with the code itself.
3. packages/opentelemetry-instrumentation-sagemaker/tests/conftest.py:50
- Draft comment:
Updated tests properly remove the deprecated parameter in instrument_legacy and instrument_with_* fixtures. This ensures consistency with the removed parameter. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining what was done in the code without providing any actionable feedback or suggestions. It doesn't ask for any specific changes or confirm any intentions, which violates the rules.
4. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:841
- Draft comment:
The init_sagemaker_instrumentor function now correctly instantiates SageMakerInstrumentor without the enrich_token_usage parameter. Ensure end-to-end consistency for users. - 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.
5. packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py:493
- Draft comment:
Ensure that any internal or external documentation references to 'enrich_token_usage' are updated to reflect its removal. - 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.
Workflow ID: wflow_WhwBPN3iFkApPecc
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/opentelemetry-instrumentation-openai/tests/traces/cassettes/test_chat/test_chat_streaming.yaml (4)
17-19: Filter volatile headers to keep the cassette stable
hostand especiallytraceparentchange between test runs. Consider adding them tovcr.use_cassette(..., filter_headers=[...])so the values are redacted. This prevents needless cassette churn and avoids leaking trace-ids into the repo.
44-239: Trim the gigantic streaming bodyThe cassette stores >200 individual SSE chunks that are irrelevant to the assertions. Keeping only the first few chunks plus the final
[DONE]token drastically reduces cassette size and diff noise while still exercising streaming logic.
248-248: Drop theDateheader
Datevaries on every recording and provides no value for assertions. Redacting it (same technique as other volatile headers) removes unnecessary diffs.
241-252: Remove internal server header
X-Powered-By: Expressdiscloses implementation details of the local mock server and provides no testing value. Safe to strip.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/opentelemetry-instrumentation-openai/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py(0 hunks)packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/config.py(0 hunks)packages/opentelemetry-instrumentation-groq/tests/traces/conftest.py(2 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/__init__.py(0 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py(0 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py(1 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py(2 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/config.py(0 hunks)packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/utils.py(0 hunks)packages/opentelemetry-instrumentation-openai/tests/conftest.py(1 hunks)packages/opentelemetry-instrumentation-openai/tests/traces/cassettes/test_chat/test_chat_streaming.yaml(2 hunks)packages/opentelemetry-instrumentation-openai/tests/traces/test_chat.py(8 hunks)packages/opentelemetry-instrumentation-openai/tests/traces/test_completions.py(2 hunks)packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.py(0 hunks)packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/config.py(0 hunks)packages/opentelemetry-instrumentation-sagemaker/tests/conftest.py(3 hunks)packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py(0 hunks)
💤 Files with no reviewable changes (9)
- packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/config.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/utils.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/config.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/init.py
- packages/traceloop-sdk/traceloop/sdk/tracing/tracing.py
- packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/init.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/config.py
- packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/init.py
- packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/init.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/opentelemetry-instrumentation-groq/tests/traces/conftest.py (1)
packages/opentelemetry-instrumentation-groq/opentelemetry/instrumentation/groq/__init__.py (1)
GroqInstrumentor(383-483)
packages/opentelemetry-instrumentation-sagemaker/tests/conftest.py (1)
packages/opentelemetry-instrumentation-sagemaker/opentelemetry/instrumentation/sagemaker/__init__.py (1)
SageMakerInstrumentor(169-213)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py (1)
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/__init__.py (1)
_set_span_stream_usage(251-272)
packages/opentelemetry-instrumentation-openai/tests/traces/test_chat.py (3)
packages/opentelemetry-instrumentation-openai/tests/traces/test_azure.py (1)
test_chat_streaming(410-463)packages/opentelemetry-instrumentation-openai/tests/conftest.py (2)
instrument_legacy(128-143)mock_openai_client(47-51)packages/opentelemetry-semantic-conventions-ai/opentelemetry/semconv_ai/__init__.py (1)
SpanAttributes(64-257)
⏰ 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.12)
- GitHub Check: Test Packages (3.9)
- GitHub Check: Test Packages (3.11)
- GitHub Check: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (22)
packages/opentelemetry-instrumentation-sagemaker/tests/conftest.py (3)
52-52: LGTM! Constructor call updated correctly.The removal of the
enrich_token_usageparameter aligns with the updatedSageMakerInstrumentorconstructor signature, which only acceptsexception_loggeranduse_legacy_attributesparameters.
67-67: LGTM! Constructor call updated correctly.The removal of the
enrich_token_usageparameter aligns with the updatedSageMakerInstrumentorconstructor signature. Theuse_legacy_attributes=Falseparameter is correctly maintained for the non-legacy instrumentation test fixture.
85-85: LGTM! Constructor call updated correctly.The removal of the
enrich_token_usageparameter aligns with the updatedSageMakerInstrumentorconstructor signature. Theuse_legacy_attributes=Falseparameter is correctly maintained for the non-legacy instrumentation test fixture.packages/opentelemetry-instrumentation-groq/tests/traces/conftest.py (3)
84-94: LGTM! Parameter removal aligns with updated constructor.The removal of
enrich_token_usage=Truefrom theGroqInstrumentor()instantiation correctly reflects the updated constructor signature where this parameter no longer exists.
97-115: LGTM! Consistent parameter removal.The
instrument_with_contentfixture has been consistently updated to remove theenrich_token_usageparameter while preserving theuse_legacy_attributes=Falseconfiguration and event logger setup.
118-136: LGTM! Parameter cleanup completed.The
instrument_with_no_contentfixture has been properly updated to remove theenrich_token_usageparameter, maintaining consistency with the other test fixtures and the updatedGroqInstrumentorconstructor.packages/opentelemetry-instrumentation-openai/tests/conftest.py (2)
46-51: LGTM! Good addition for test isolation.The new
mock_openai_clientfixture provides a consistent way to test with a local mock server, avoiding real API calls and improving test reliability.
132-135: Parameter removal correctly implemented.The removal of
enrich_token_usagefrom the OpenAI instrumentor constructor aligns with the PR objective to simplify token calculation logic.packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/chat_wrappers.py (1)
533-540: Simplified token usage logic correctly implemented.The change to rely solely on API-provided usage data simplifies the logic and removes the complexity of local token counting. The function now has a clear single source of truth for token metrics.
packages/opentelemetry-instrumentation-openai/opentelemetry/instrumentation/openai/shared/completion_wrappers.py (2)
232-244: Consistent simplification of token usage logic.The function now uses only API-provided usage data, maintaining consistency with the chat wrapper implementation and the overall PR objective to simplify token calculation.
255-257: Good addition to preserve streaming usage data.The logic to capture usage information from stream chunks ensures token usage data is available for the simplified token usage calculation when streaming responses are used.
packages/opentelemetry-instrumentation-openai/tests/traces/test_completions.py (3)
348-394: Test updates appropriately reflect the instrumentation changes.The use of
mock_openai_client, updated API base URL expectations, and conditional token usage assertions correctly align with the simplified token calculation logic. The removal of environment variable manipulation simplifies the test setup.
419-430: Robust conditional token usage testing.The conditional assertions for token usage provide good test coverage while accounting for the fact that token usage data may not always be available from the API response. This makes the tests more resilient.
481-493: Consistent test pattern maintained across streaming tests.The conditional token usage assertions are consistently applied across all streaming tests, with clear comments explaining the dependency on API support. This ensures robust test coverage under the new token usage approach.
packages/opentelemetry-instrumentation-openai/tests/traces/cassettes/test_chat/test_chat_streaming.yaml (1)
13-13: Verify the recordedcontent-lengthThe body has clearly changed since the previous recording, yet
content-lengthis still hard-coded to123. If VCR is configured to validate request headers this will break replay. Please re-record or delete the header.packages/opentelemetry-instrumentation-openai/tests/traces/test_chat.py (7)
584-584: LGTM - Switch to mock client for testingThe change to use
mock_openai_clientinstead ofopenai_clientis appropriate for this test, providing better isolation and consistency in the test environment.
609-611: LGTM - Updated API base URL for mock clientThe assertion correctly reflects the mock client's localhost base URL configuration, maintaining test accuracy.
615-616: LGTM - More robust event count assertionThe updated comment and assertion appropriately account for potential differences in mock server behavior while still validating the core functionality (presence of events).
626-628: LGTM - Added clarifying comment for token usage validationThe comment helps explain the validation logic for token usage attributes, improving code readability.
630-632: LGTM - Updated response ID for mock serverThe response ID correctly reflects the mock server's response data, maintaining test accuracy.
678-680: LGTM - Conditional token usage validation aligns withenrich_token_usageremovalThe conditional token usage assertions are appropriate given the removal of the
enrich_token_usageparameter. The instrumentation now relies solely on token usage data provided by the API response, which may not always be present.This represents a behavioral change where token usage attributes are no longer guaranteed to be available in all scenarios, but the validation logic correctly handles both cases.
Also applies to: 741-741, 749-750, 813-814, 865-866, 934-935
741-741: LGTM - Clarifying comment about optional token usageThe comment appropriately explains that token usage attributes are optional and API-dependent, helping future maintainers understand the conditional assertion logic.
There was a problem hiding this comment.
Have you remove the tiktoken requirement from pyproject.toml?
There was a problem hiding this comment.
I've only removed the tiktoken dependency from OpenAI.
Since I haven't modified the other instrumentations, their tiktoken dependencies have been kept to avoid other potential issues.
…token calculation (#3205) Co-authored-by: Nir Gazit <nirga@users.noreply.github.com>
feat(instrumentation): ...orfix(instrumentation): ....Fix #2898 and related to #3142 (comment) and #3168. Ref https://platform.openai.com/docs/api-reference/chat/object#chat/object-usage
I've removed the
enrich_token_usageparameter from OpenAI and also removed it from Groq and Sagemaker, as it never had any practical effect. I preserved it in Anthropic(the same applies to Bedrock) because it controls the calculation logic foranthropic.Anthropic().messages.count_tokens.Removed:
Reserve:
Important
Remove
enrich_token_usageparameter from OpenAI, Groq, and Sagemaker instrumentors, simplifying token usage calculation.enrich_token_usageparameter fromOpenAI,Groq, andSageMakerinstrumentors.enrich_token_usageforAnthropicandBedrockdue to specific logic requirements.test_chat.pyandtest_completions.pyto reflect removal ofenrich_token_usage.test_chat_streamingandtest_completion_streamingto check token usage only if provided by API.init_openai_instrumentorandinit_sagemaker_instrumentorintracing.pyto removeenrich_token_usageparameter.This description was created by
for d77d742. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
enrich_token_usageconfiguration option across OpenAI, SageMaker, and Groq instrumentations.enrich_token_usageand related logic.Tests