fix(crewai): fix unpack error when metrics are disabled #3345
Conversation
WalkthroughInitialized metric histogram variables to None before conditional metrics setup, simplified assignment from _create_metrics(meter) to two variables, and removed redundant else branch. Downstream usage remains unchanged, with wrappers receiving possibly None histograms. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Instrumentation.init
participant Meter as Meter
participant Metrics as _create_metrics
participant Wrappers as Wrappers
Init->>Init: token_histogram = None<br/>duration_histogram = None
alt metrics enabled
Init->>Meter: get meter
Init->>Metrics: _create_metrics(meter)
Metrics-->>Init: token_histogram, duration_histogram
else metrics disabled
Note over Init: Histograms remain None
end
Init->>Wrappers: pass token_histogram, duration_histogram
Wrappers-->>Wrappers: Handle None or histogram instances
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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 151add2 in 34 seconds. Click for details.
- Reviewed
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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-crewai/opentelemetry/instrumentation/crewai/instrumentation.py:30
- Draft comment:
Good fix: initializing token_histogram and duration_histogram as None avoids unpack errors when metrics are disabled. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_dde2RSYqhqBTKsC6
You can customize 
![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
🧹 Nitpick comments (4)
packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py (4)
62-63: Annotate histograms as Optional[Histogram] to match runtime None valuesNow that histograms can be None, the type hints should reflect Optional[Histogram] to keep static analysis and IDEs honest. While runtime behavior is fine, this avoids misleading signatures and helps Flake8/mypy users.
Apply the following minimal diff:
-from typing import Collection +from typing import Collection, Optional @@ def with_tracer_wrapper(func): """Helper for providing tracer for wrapper functions.""" - def _with_tracer(tracer, duration_histogram, token_histogram): + def _with_tracer(tracer: Tracer, + duration_histogram: Optional[Histogram], + token_histogram: Optional[Histogram]): def wrapper(wrapped, instance, args, kwargs): return func(tracer, duration_histogram, token_histogram, wrapped, instance, args, kwargs) return wrapper return _with_tracer @@ -@with_tracer_wrapper -def wrap_kickoff(tracer: Tracer, duration_histogram: Histogram, token_histogram: Histogram, +@with_tracer_wrapper +def wrap_kickoff(tracer: Tracer, + duration_histogram: Optional[Histogram], + token_histogram: Optional[Histogram], wrapped, instance, args, kwargs): @@ -@with_tracer_wrapper -def wrap_agent_execute_task(tracer, duration_histogram, token_histogram, wrapped, instance, args, kwargs): +@with_tracer_wrapper +def wrap_agent_execute_task(tracer: Tracer, + duration_histogram: Optional[Histogram], + token_histogram: Optional[Histogram], + wrapped, instance, args, kwargs): @@ -@with_tracer_wrapper -def wrap_task_execute(tracer, duration_histogram, token_histogram, wrapped, instance, args, kwargs): +@with_tracer_wrapper +def wrap_task_execute(tracer: Tracer, + duration_histogram: Optional[Histogram], + token_histogram: Optional[Histogram], + wrapped, instance, args, kwargs): @@ -@with_tracer_wrapper -def wrap_llm_call(tracer, duration_histogram, token_histogram, wrapped, instance, args, kwargs): +@with_tracer_wrapper +def wrap_llm_call(tracer: Tracer, + duration_histogram: Optional[Histogram], + token_histogram: Optional[Histogram], + wrapped, instance, args, kwargs):Also applies to: 89-90, 129-130, 151-152, 54-57, 3-3
181-183: Make TRACELOOP_METRICS_ENABLED parsing more robustCurrent parsing only recognizes "true". Consider supporting common truthy/falsey variants ("1/0", "yes/no", "on/off") while keeping the default enabled. This reduces surprises in different deployment setups.
-def is_metrics_enabled() -> bool: - return (os.getenv("TRACELOOP_METRICS_ENABLED") or "true").lower() == "true" +def is_metrics_enabled() -> bool: + val = os.getenv("TRACELOOP_METRICS_ENABLED") + if val is None: + return True # default on + return val.strip().lower() in {"1", "true", "yes", "on"}
101-117: Avoid duplicate get_summary() calls when recording tokensMinor nit:
instance._token_process.get_summary()is invoked twice. Cache once to avoid repeated work or potential side effects in client implementations.- if token_histogram: - token_histogram.record( - instance._token_process.get_summary().prompt_tokens, + if token_histogram: + summary = instance._token_process.get_summary() + token_histogram.record( + summary.prompt_tokens, attributes={ SpanAttributes.LLM_SYSTEM: "crewai", SpanAttributes.LLM_TOKEN_TYPE: "input", SpanAttributes.LLM_RESPONSE_MODEL: str(instance.llm.model), } ) - token_histogram.record( - instance._token_process.get_summary().completion_tokens, + token_histogram.record( + summary.completion_tokens, attributes={ SpanAttributes.LLM_SYSTEM: "crewai", SpanAttributes.LLM_TOKEN_TYPE: "output", SpanAttributes.LLM_RESPONSE_MODEL: str(instance.llm.model), }, )
30-33: Add a regression test for “metrics disabled” initialization pathTo prevent recurrence, add a small test that sets
TRACELOOP_METRICS_ENABLED=falseand asserts_instrument()completes without raising and that no metrics are recorded. Happy to open a follow-up PR if helpful.Example test (pytest-style):
import os from opentelemetry.instrumentation.crewai.instrumentation import CrewAIInstrumentor def test_metrics_disabled_initialization(monkeypatch): monkeypatch.setenv("TRACELOOP_METRICS_ENABLED", "false") inst = CrewAIInstrumentor() # Should not raise inst._instrument(tracer_provider=None, meter_provider=None) # Optionally, assert wrappers exist by checking wrapt has applied wrappers # and that calling them with None histograms is a no-op.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py(1 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-crewai/opentelemetry/instrumentation/crewai/instrumentation.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: Test Packages (3.10)
- GitHub Check: Build Packages (3.11)
- GitHub Check: Lint
🔇 Additional comments (1)
packages/opentelemetry-instrumentation-crewai/opentelemetry/instrumentation/crewai/instrumentation.py (1)
30-33: Fix for unpack error when metrics are disabled — LGTMPre-initializing the histograms to None and switching to a two-value assignment from
_create_metrics(meter)correctly avoids the prior “too many values to unpack” failure when metrics are off. This keeps downstream wrappers stable since they already guard on the presence of the histograms. Nice, focused fix.
2 participants
feat(instrumentation): ...orfix(instrumentation): ....This PR fixes an initialization error in the CrewAI instrumentor that occurs when metrics are disabled:
Important
Fixes unpack error in
CrewAIInstrumentor._instrument()when metrics are disabled by initializing histograms toNone.CrewAIInstrumentor._instrument()when metrics are disabled by initializingtoken_histogramandduration_histogramtoNone._instrument()ininstrumentation.pyto handle cases whereis_metrics_enabled()returnsFalse.This description was created by
for 151add2. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Bug Fixes
Refactor