fix otel duplicates #93
Conversation
WalkthroughReplaces legacy OpenTelemetry exporters/providers and signal handling with an optional Traceloop-based instrumentation path in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant OTEL as agentuity.otel
participant TL as Traceloop
participant Log as logger
App->>OTEL: init(config)
OTEL->>OTEL: build app_name, headers, resource_attributes
alt Traceloop present and config valid
OTEL->>TL: Traceloop.init(app_name, api_endpoint, headers, resource_attributes, disable_batch?, telemetry_enabled=False)
TL-->>OTEL: success
OTEL-->>App: return None
else Traceloop missing or init error
OTEL->>Log: warn("Instrumentation disabled or initialization failed")
OTEL-->>App: return None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agentuity/otel/__init__.py (1)
14-15: Avoid mutable default argument; normalize config.Using {} as a default is error-prone. Prefer None and coalesce.
-def init(config: Optional[Dict[str, str]] = {}): +def init(config: Optional[Dict[str, str]] = None): + config = config or {}
🧹 Nitpick comments (4)
agentuity/otel/__init__.py (4)
15-17: Harden truthy parsing for AGENTUITY_OTLP_DISABLED.Accept common truthy values (true/1/yes/on); current check is too strict.
- if os.environ.get("AGENTUITY_OTLP_DISABLED", "false") == "true": + if str(os.environ.get("AGENTUITY_OTLP_DISABLED", "false")).strip().lower() in ("1", "true", "yes", "on"):
45-48: Normalize devmode parsing.Same issue: accept broader truthy values.
- devmode = ( - config.get("devmode", os.environ.get("AGENTUITY_SDK_DEV_MODE", "false")) - == "true" - ) + devmode = str(config.get("devmode", os.environ.get("AGENTUITY_SDK_DEV_MODE", "false"))).strip().lower() in ("1", "true", "yes", "on")
65-66: Minor: simplify headers construction.You already early-return when bearer_token is missing; conditional is unnecessary.
- headers = {"Authorization": f"Bearer {bearer_token}"} if bearer_token else {} + headers = {"Authorization": f"Bearer {bearer_token}"}
84-86: Verify attribute key conventions.OTel attribute keys generally follow namespace.key with [a–z0–9_.-]. The custom keys with '@' and '/' may be dropped by some SDKs/exporters. If Traceloop forwards to OTEL resources, consider renaming to agentuity.org_id, agentuity.project_id, etc., and keep custom keys under a single namespace.
I can propose a safe key migration plan if needed.
📜 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
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
agentuity/otel/__init__.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Tests
agentuity/otel/__init__.py
[error] 1-1: Missing TracerProvider attribute in agentuity.otel module. AttributeError: TracerProvider not found when patching in tests.
🔇 Additional comments (3)
agentuity/otel/__init__.py (3)
92-95: Confirm Traceloop flags.disable_batch=devmode and telemetry_enabled=False look intentional; just confirm desired behavior in CI/prod vs local dev.
If prod should batch, this is correct; if not, flip accordingly.
103-103: Explicit None return is fine.Clear and consistent with the new init semantics.
1-10: Add backward-compatible TracerProvider re-exportRe-export TracerProvider from opentelemetry.sdk.trace in agentuity/otel/init.py, include it in all, and fall back to None to satisfy tests patching agentuity.otel.TracerProvider.
| from opentelemetry.propagate import set_global_textmap | ||
| from .logfilter import ModuleFilter | ||
| from opentelemetry.trace.propagation.tracecontext import TraceContextTextMapPropagator | ||
| from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make OTEL an optional dependency (protect import).
Importing SERVICE_NAME/SERVICE_VERSION at module import time breaks environments without OpenTelemetry installed. Guard it and fall back to string keys.
-from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION
+try:
+ from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION
+except Exception:
+ # Fallback to canonical attribute names when OTEL isn't installed
+ SERVICE_NAME = "service.name"
+ SERVICE_VERSION = "service.version"📝 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.
| from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION | |
| try: | |
| from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION | |
| except Exception: | |
| # Fallback to canonical attribute names when OTEL isn't installed | |
| SERVICE_NAME = "service.name" | |
| SERVICE_VERSION = "service.version" |
🤖 Prompt for AI Agents
In agentuity/otel/__init__.py around line 5, importing SERVICE_NAME and
SERVICE_VERSION from opentelemetry at module import time will raise when
OpenTelemetry isn't installed; wrap the import in a try/except ImportError and
on ImportError assign SERVICE_NAME and SERVICE_VERSION to the literal string
keys you expect to use (e.g. "service.name" and "service.version"), ensuring the
module still exposes those names for downstream code without requiring the
opentelemetry dependency.
agentuity/otel/__init__.py
Outdated
| project_name = config.get("project_name", "") | ||
| agent_name = config.get("agent_name", "") | ||
| app_name = f"{project_name}:{agent_name}" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t clobber a provided app_name; avoid ":" when names are empty.
Only compose project:agent when both exist; otherwise keep the configured/env app_name.
- project_name = config.get("project_name", "")
- agent_name = config.get("agent_name", "")
- app_name = f"{project_name}:{agent_name}"
+ project_name = config.get("project_name", "")
+ agent_name = config.get("agent_name", "")
+ if project_name and agent_name:
+ app_name = f"{project_name}:{agent_name}"
+ # else: preserve previously resolved app_name from config/envAlso applies to: 49-54
🤖 Prompt for AI Agents
In agentuity/otel/__init__.py around lines 61-64 (and similarly lines 49-54),
the current logic unconditionally overwrites any provided app_name and builds
app_name as "project:agent" even when one or both parts are empty; change it to
first respect an explicitly configured/app_name from config or env and only
compose "project:agent" when both project_name and agent_name are non-empty,
otherwise use whichever non-empty value exists (or keep empty if none);
implement simple conditional logic: if config contains app_name use it, else if
both project_name and agent_name non-empty set f"{project_name}:{agent_name}",
else set app_name to the non-empty one or empty string.
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 (1)
tests/otel/test_init.py (1)
5-5: Fix lint failure: remove unusedCompressionimport.Ruff F401 is breaking the pipeline. This import isn’t used anywhere in the tests.
-from opentelemetry.exporter.otlp.proto.http.trace_exporter import Compression
🧹 Nitpick comments (4)
tests/otel/test_init.py (4)
80-87: Assert no positional args and make header assertion less brittle.Traceloop may add headers later (e.g., Content-Type). Asserting only Authorization avoids fragile equality; also assert zero positional args.
- args, kwargs = mock_traceloop_init.call_args - assert kwargs["api_endpoint"] == "https://test.com" - assert kwargs["headers"] == {"Authorization": "Bearer test_token"} - assert kwargs["resource_attributes"][SERVICE_NAME] == "test_service" - assert kwargs["resource_attributes"][SERVICE_VERSION] == "1.0.0" + args, kwargs = mock_traceloop_init.call_args + assert args == () + assert kwargs["api_endpoint"] == "https://test.com" + assert kwargs["headers"].get("Authorization") == "Bearer test_token" + res = kwargs["resource_attributes"] + assert res[SERVICE_NAME] == "test_service" + assert res[SERVICE_VERSION] == "1.0.0"
14-26: Optional: add a case-insensitivity/variant test for disable flag.If init treats “true”, “TRUE”, or “1” as disabled, add a parametric test to cover those inputs.
65-87: Optional: add tests for env-var-only config and idempotency (dupe prevention).Given the PR goal (eliminate duplicate telemetry), consider:
- A test where endpoint/token/name/version come solely from env vars.
- A test that calling init twice does not invoke Traceloop.init twice (if intended).
Example tests to add:
def test_init_with_env_vars_only(): with patch.dict(os.environ, { "AGENTUITY_OTLP_DISABLED": "false", "AGENTUITY_OTLP_URL": "https://env.test", "AGENTUITY_OTLP_BEARER_TOKEN": "env_token", "AGENTUITY_SERVICE_NAME": "env_service", "AGENTUITY_SERVICE_VERSION": "9.9.9", }, clear=False), patch("traceloop.sdk.Traceloop.init") as m, patch("agentuity.otel.logger"): from agentuity import otel as otel_mod # pass empty config to force env usage assert otel_mod.init({}) is None m.assert_called_once() _, kwargs = m.call_args assert kwargs["api_endpoint"] == "https://env.test" assert kwargs["headers"].get("Authorization") == "Bearer env_token" res = kwargs["resource_attributes"] assert res[SERVICE_NAME] == "env_service" assert res[SERVICE_VERSION] == "9.9.9" def test_init_is_idempotent_when_called_twice(): cfg = { "endpoint": "https://test.com", "bearer_token": "test_token", "service_name": "svc", "service_version": "1.2.3", } with patch("traceloop.sdk.Traceloop.init") as m, patch("agentuity.otel.logger"): from agentuity import otel as otel_mod otel_mod.init(cfg) otel_mod.init(cfg) # Adjust expectation to desired behavior: assert m.call_count == 1 # or == 2 if intentional
7-8: Nit: avoid leaking fake module insys.modules.Setting
sys.modules["openlit"]at import time can leak across other tests. Preferpatch.dict(sys.modules, {"openlit": MagicMock()})scoped to specific tests or a fixture that restores state.
📜 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
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/otel/test_init.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Run Tests
tests/otel/test_init.py
[error] 5-5: F401: 'Compression' imported but unused in tests/otel/test_init.py:5. Remove unused import. Step 'make lint' failed with exit code 1. 1 fixable with the --fix option.
🪛 GitHub Actions: Build
tests/otel/test_init.py
[error] 5-5: Ruff check failed: F401 'Compression' imported from opentelemetry.exporter.otlp.proto.http.trace_exporter but unused.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/otel/test_init.py (7)
75-76: Patch the symbol where it’s used to make the test resilient.Patch
agentuity.otel.Traceloop.init(the imported symbol) instead of the third-party module path. This avoids breakage if import style changes.Apply:
- with ( - patch("traceloop.sdk.Traceloop.init") as mock_traceloop_init, + with ( + patch("agentuity.otel.Traceloop.init") as mock_traceloop_init, patch("agentuity.otel.logger"), ):
83-87: Slightly harden the assertion: enforce kwargs-only call.Also assert no positional args were used to keep the call explicit.
Apply:
- args, kwargs = mock_traceloop_init.call_args + args, kwargs = mock_traceloop_init.call_args + assert args == () assert kwargs["api_endpoint"] == "https://test.com" assert kwargs["headers"] == {"Authorization": "Bearer test_token"} assert kwargs["resource_attributes"][SERVICE_NAME] == "test_service" assert kwargs["resource_attributes"][SERVICE_VERSION] == "1.0.0"
4-4: Decouple tests from OTel SDK by inlining the attribute keys.Using string literals avoids a hard dependency on
opentelemetryin tests and prevents version/API drift from breaking tests.Apply:
-from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION +# Avoid hard dependency on OTel SDK constants in tests +SERVICE_NAME = "service.name" +SERVICE_VERSION = "service.version"
14-25: Also assert Traceloop is not initialized when disabled.Prevents regressions where init happens despite the guard.
Apply:
- with ( - patch.dict(os.environ, {"AGENTUITY_OTLP_DISABLED": "true"}), - patch("agentuity.otel.logger", mock_logger), - ): + with ( + patch.dict(os.environ, {"AGENTUITY_OTLP_DISABLED": "true"}), + patch("agentuity.otel.logger", mock_logger), + patch("agentuity.otel.Traceloop.init") as mock_traceloop_init, + ): result = init() assert result is None mock_logger.warning.assert_called_once_with( "OTLP disabled, skipping initialization" ) + mock_traceloop_init.assert_not_called()
27-41: Guard against accidental init when endpoint is missing.Mirror the disabled-case check to ensure no Traceloop initialization occurs.
Apply:
- with ( - patch.dict(os.environ, {"AGENTUITY_OTLP_DISABLED": "false"}), - patch("agentuity.otel.logger", mock_logger), - ): + with ( + patch.dict(os.environ, {"AGENTUITY_OTLP_DISABLED": "false"}), + patch("agentuity.otel.logger", mock_logger), + patch("agentuity.otel.Traceloop.init") as mock_traceloop_init, + ): if "AGENTUITY_OTLP_URL" in os.environ: del os.environ["AGENTUITY_OTLP_URL"] result = init({}) assert result is None mock_logger.warning.assert_called_once_with( "No endpoint found, skipping OTLP initialization" ) + mock_traceloop_init.assert_not_called()
43-63: Guard against accidental init when bearer token is missing.Same rationale as above.
Apply:
- with ( + with ( patch.dict( os.environ, { "AGENTUITY_OTLP_DISABLED": "false", "AGENTUITY_OTLP_URL": "https://test.com", }, ), - patch("agentuity.otel.logger", mock_logger), + patch("agentuity.otel.logger", mock_logger), + patch("agentuity.otel.Traceloop.init") as mock_traceloop_init, ): if "AGENTUITY_OTLP_BEARER_TOKEN" in os.environ: del os.environ["AGENTUITY_OTLP_BEARER_TOKEN"] result = init({}) assert result is None mock_logger.warning.assert_called_once_with( "No bearer token found, skipping OTLP initialization" ) + mock_traceloop_init.assert_not_called()
88-88: Add a positive “env-only” test to cover default config path.Covers the scenario where config is omitted and env vars drive initialization (helps detect duplicate-init regressions).
Apply:
@@ def test_init_with_config(self): """Test init with valid configuration.""" @@ assert kwargs["resource_attributes"][SERVICE_VERSION] == "1.0.0" + def test_init_from_env_only(self): + """Test init with only environment variables (no explicit config).""" + with ( + patch.dict( + os.environ, + { + "AGENTUITY_OTLP_DISABLED": "false", + "AGENTUITY_OTLP_URL": "https://test.com", + "AGENTUITY_OTLP_BEARER_TOKEN": "test_token", + }, + ), + patch("agentuity.otel.Traceloop.init") as mock_traceloop_init, + patch("agentuity.otel.logger"), + ): + result = init() + assert result is None + mock_traceloop_init.assert_called_once() + _, kwargs = mock_traceloop_init.call_args + assert kwargs["api_endpoint"] == "https://test.com" + assert kwargs["headers"] == {"Authorization": "Bearer test_token"}
📜 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
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/otel/test_init.py(2 hunks)
🔇 Additional comments (2)
tests/otel/test_init.py (2)
5-5: No-op change.Whitespace-only; nothing to review.
80-80: LGTM: return value assertion matches the new API.
agentuity/otel/__init__.py
Outdated
| # Build app name from project and agent info if available | ||
| project_name = config.get("project_name", "") | ||
| agent_name = config.get("agent_name", "") | ||
| app_name = f"{project_name}:{agent_name}" |
There was a problem hiding this comment.
otel is global so should just be project name?
There was a problem hiding this comment.
we should make the format for these the exact same as previous version
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 (1)
agentuity/otel/__init__.py (1)
27-29: Treat empty strings as missing bearer tokens.Current check permits empty strings; tighten to a truthiness check and update the message to reflect telemetry vs. OTLP.
- if bearer_token is None: - logger.warning("No bearer token found, skipping OTLP initialization") + if not bearer_token: + logger.warning("No bearer token found, skipping telemetry initialization") return None
♻️ Duplicate comments (1)
agentuity/otel/__init__.py (1)
5-5: Guard OTEL constants import; fall back to string keys when OTEL isn’t installed.Importing from opentelemetry at module import time will crash in environments without OTEL. Wrap it and default to canonical keys.
-from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION +try: + from opentelemetry.sdk.resources import SERVICE_NAME, SERVICE_VERSION +except ImportError: + # Fallback when OpenTelemetry is not installed + SERVICE_NAME = "service.name" + SERVICE_VERSION = "service.version"
🧹 Nitpick comments (4)
agentuity/otel/__init__.py (4)
14-14: Avoid a mutable default argument for config.Use None and normalize inside to prevent accidental shared state.
-def init(config: Optional[Dict[str, str]] = {}): +def init(config: Optional[Dict[str, str]] = None): + if config is None: + config = {}
60-61: Simplify header construction (after tightening the token check).Redundant conditional can be removed once empty tokens are rejected.
- headers = {"Authorization": f"Bearer {bearer_token}"} if bearer_token else {} + headers = {"Authorization": f"Bearer {bearer_token}"}
45-48: Parse boolean flags robustly (supports bool and common truthy strings).String-only comparison is brittle. Normalize to a proper boolean for devmode.
- devmode = ( - config.get("devmode", os.environ.get("AGENTUITY_SDK_DEV_MODE", "false")) - == "true" - ) + devmode = _to_bool( + config.get("devmode", os.environ.get("AGENTUITY_SDK_DEV_MODE", "false")) + )Add this helper near the top of the file:
def _to_bool(value: object) -> bool: if isinstance(value, bool): return value return str(value).strip().lower() in {"1", "true", "t", "yes", "y", "on"}
62-81: Optional: De-duplicate “env” resource attribute
No code paths consume the short “env” key (only “@agentuity/env” is used downstream), so you may drop or realign the"env": "dev"/"production"entry inagentuity/otel/__init__.pyif you want a single canonical field.
📜 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
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
agentuity/otel/__init__.py(4 hunks)
🔇 Additional comments (1)
agentuity/otel/__init__.py (1)
87-90: Verify Tracelooptelemetry_enabled=Falsesemantics.Confirm this disables vendor meta-telemetry only and does not suppress trace/span export, avoiding a “fix” that drops telemetry entirely.
Refactor OTEL initialization to fix duplicate telemetry
Refactored the OpenTelemetry (OTEL) initialization to use
Traceloop.initexclusively, removing the manual OTEL setup. This resolves an issue where telemetry data was being sent twice.Traceloop.init.Summary by CodeRabbit
New Features
Refactor
Improvements
Tests