feat: Add user-provided OpenTelemetry configuration support #95
Conversation
- Add UserOpenTelemetryConfig dataclass for parsing user OTEL config from AGENTUITY_USER_OTEL_CONF env var - Implement create_user_logger_provider() to create user OTLP logger providers - Add multi-delegate logging system that sends logs to both Agentuity and user endpoints - Enhance create_logger() to automatically add MultiDelegateHandler when user providers exist - Add proper shutdown handling for user logger providers with flush and cleanup - Implement graceful error handling that doesn't break existing functionality - Add comprehensive test coverage with 25+ new tests - Maintain full backward compatibility with existing OTEL functionality - Add ruff as dev dependency for code linting consistency This implementation mirrors the functionality from the JavaScript SDK PR #176, allowing users to send agent logs to their own OTLP endpoints alongside the default Agentuity telemetry collection. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-1dd20dcd-70fb-4f19-9de2-1984bb3b9a05
WalkthroughAdds optional user-provided OpenTelemetry support and a multi-delegate logging path: parse env config, create/register OTLP logger providers, forward Python LogRecords to user providers via a MultiDelegateHandler, integrate provider lifecycle into init() and server shutdown, add tests, and introduce a dev dependency group. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Init as otel.init()
participant Logger as Python Logger
participant MDH as MultiDelegateHandler
participant Emit as emit_to_user_providers()
participant Provider as User OTLP Provider
participant SDK as OpenTelemetry SDK
Note over Init,Provider: init() may parse env and create provider
App->>Init: start / init()
Init->>Provider: create_user_logger_provider() [optional]
Provider-->>Init: registered
Logger->>MDH: create_logger() attaches MDH
Logger->>Logger: logger.handle(LogRecord)
Logger->>MDH: MDH.emit(record)
MDH->>Emit: emit_to_user_providers(record)
Emit->>Emit: map level -> SeverityNumber\nmerge attributes, timestamp (ns)
Emit->>Provider: provider.get_logger(name) -> emit(event)
Provider->>SDK: OTLP export
SDK-->>Provider: ack / error
alt Provider raises
Provider--X Emit: exception
Emit-->>Logger: warning logged, continue
else Success
Provider-->>Emit: success
end
App->>Init: shutdown
Init->>Provider: shutdown()/force-close
Provider-->>Init: cleared
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
pyproject.toml (1)
49-53: Avoid duplicate dev deps and align ruff versions.You now declare ruff in two places: [project.optional-dependencies].dev ("ruff>=0.1.0") and [dependency-groups].dev ("ruff>=0.11.11"). This can drift and confuse tooling (uv vs pip extras). Pick one source of truth or keep both but pin the same spec.
Apply either:
- Remove ruff from [project.optional-dependencies].dev and rely on [dependency-groups] for uv-only workflows; or
- Keep extras.dev and drop [dependency-groups]; or
- Keep both but use identical constraints.
Also, please verify that "0.11.11" is an intended version and actually exists for ruff.
agentuity/otel/__init__.py (3)
16-26: Dataclass defaults: prefer concrete dicts over Optionals when you set {} later.parse_user_otel_config returns {} for resource_attributes and headers, while dataclass fields are Optional. Make them Dict[...] = field(default_factory=dict) to simplify consumers and types.
-@dataclass -class UserOpenTelemetryConfig: +@dataclass +class UserOpenTelemetryConfig: """Configuration for user-provided OpenTelemetry setup.""" endpoint: str service_name: str - resource_attributes: Optional[Dict[str, Any]] = None - headers: Optional[Dict[str, str]] = None + resource_attributes: Dict[str, Any] = field(default_factory=dict) + headers: Dict[str, str] = field(default_factory=dict)
28-56: Normalize and validate headers/attributes on parse.User-provided headers/attributes can contain non-string/invalid types. Normalize header keys to lowercase and coerce non-primitive attribute values to strings to avoid exporter errors.
- return UserOpenTelemetryConfig( + headers = {str(k).lower(): str(v) for k, v in (config_data.get("headers", {}) or {}).items()} + attrs = config_data.get("resourceAttributes", {}) or {} + safe_attrs = {str(k): (v if isinstance(v, (str, int, float, bool)) else str(v)) for k, v in attrs.items()} + return UserOpenTelemetryConfig( endpoint=config_data["endpoint"], service_name=config_data["serviceName"], - resource_attributes=config_data.get("resourceAttributes", {}), - headers=config_data.get("headers", {}) + resource_attributes=safe_attrs, + headers=headers )
63-111: User logger provider creation looks solid; consider compression and timeouts.Recommend enabling gzip and making timeouts configurable to harden network IO without changing defaults.
- exporter = OTLPLogExporter( + exporter = OTLPLogExporter( endpoint=logs_url, headers=user_config.headers or {}, - timeout=10, + timeout=int(os.environ.get("AGENTUITY_USER_OTEL_TIMEOUT", "10")), + compression=os.environ.get("AGENTUITY_USER_OTEL_COMPRESSION", "gzip"), )agentuity/otel/logger.py (2)
9-14: Dedup and type the provider registry.Prevent duplicate registrations and add a type for better tooling. Optional: guard with a lock or id(provider) set if concurrency matters.
-from typing import List +from typing import List, Any, Set +import threading @@ -_user_logger_providers: List = [] +_user_logger_providers: List[Any] = [] +_user_provider_ids: Set[int] = set() +_user_providers_lock = threading.Lock() @@ def add_user_logger_provider(provider): """Add a user logger provider to the multi-delegate system.""" - global _user_logger_providers - _user_logger_providers.append(provider) + global _user_logger_providers, _user_provider_ids + with _user_providers_lock: + pid = id(provider) + if pid in _user_provider_ids: + return + _user_logger_providers.append(provider) + _user_provider_ids.add(pid) logging.info("Added user logger provider to multi-delegate system")
16-63: Make emission robust: snapshot providers, pre-import SeverityNumber, and sanitize attributes.
- Snapshot providers to avoid concurrent modification during iteration.
- Import SeverityNumber once at module scope.
- Only pass OTEL-supported attribute types; coerce others to str to avoid exporter errors.
-from opentelemetry._logs import SeverityNumber +# at top-level +from opentelemetry._logs import SeverityNumber +_SEVERITY_MAP = { + logging.DEBUG: SeverityNumber.DEBUG, + logging.INFO: SeverityNumber.INFO, + logging.WARNING: SeverityNumber.WARN, + logging.ERROR: SeverityNumber.ERROR, + logging.CRITICAL: SeverityNumber.FATAL, +} @@ - for provider in _user_logger_providers: + # Snapshot to avoid mutation during iteration + providers = list(_user_logger_providers) + for provider in providers: @@ - # Convert log level to OpenTelemetry severity - from opentelemetry._logs import SeverityNumber - - severity_map = { - logging.DEBUG: SeverityNumber.DEBUG, - logging.INFO: SeverityNumber.INFO, - logging.WARNING: SeverityNumber.WARN, - logging.ERROR: SeverityNumber.ERROR, - logging.CRITICAL: SeverityNumber.FATAL, - } - - severity = severity_map.get(record.levelno, SeverityNumber.INFO) + severity = _SEVERITY_MAP.get(record.levelno, SeverityNumber.INFO) @@ - for key, value in record.__dict__.items(): + for key, value in record.__dict__.items(): if not key.startswith('_') and key not in ['name', 'msg', 'args', 'levelname', 'levelno', 'pathname', 'filename', 'module', 'lineno', 'funcName', 'created', 'msecs', 'relativeCreated', 'thread', 'threadName', 'processName', 'process']: - attributes[key] = value + if isinstance(value, (str, int, float, bool)): + attributes[key] = value + else: + attributes[key] = str(value)tests/otel/test_user_otel.py (2)
125-138: Narrow the import-error patch to reduce blast radius.Patching builtins.import can mask unrelated imports. Prefer targeting the exporter import path.
-with patch('builtins.__import__', side_effect=ImportError("No module")): +with patch('opentelemetry.exporter.otlp.proto.http._log_exporter.OTLPLogExporter', side_effect=ImportError("No module")):
190-232: Add assertions for processor/exporter shutdown once implemented.If you adopt flushing processor/exporter in shutdown, add asserts for both to be called, and keep provider shutdown assert.
- mock_provider.force_flush.assert_called_once() - mock_provider.shutdown.assert_called_once() + mock_provider.shutdown.assert_called_once() + # New if implemented + result = agentuity.otel._user_logger_provider + # Example if you expose processor/exporter mocks: + # mock_processor.force_flush.assert_called_once() + # mock_exporter.shutdown.assert_called_once()tests/otel/test_user_logger.py (5)
15-21: Consider using a fixture or public API instead of directly mutating global state.The tests directly call
.clear()on the module-level_user_logger_providerslist. This creates tight coupling to internal implementation details and makes the tests fragile to refactoring. If_user_logger_providersis later changed to a different data structure or moved behind an API, all these tests will break.Consider one of these approaches:
- Add a public
clear_user_logger_providers()function to the module- Use a pytest fixture that automatically resets state:
@pytest.fixture(autouse=True) def reset_user_logger_providers(): _user_logger_providers.clear() yield _user_logger_providers.clear()This pattern repeats in all test classes (lines 182-187, 211-217), so fixing it once would improve all tests.
48-60: Inconsistent LogRecord creation across tests.This test creates a
LogRecordwithout explicitly settingmoduleandfuncNameattributes, but other tests (e.g., lines 79-80) manually set these attributes after creation. This inconsistency could mask bugs if the code under test depends on these attributes being present.Consider creating a helper function to create fully-populated test LogRecords:
def create_test_log_record(level=logging.INFO, msg="Test message", **extra_attrs): record = logging.LogRecord( name="test", level=level, pathname="/path/test.py", lineno=42, msg=msg, args=(), exc_info=None ) record.module = "test" record.funcName = "test_function" for key, value in extra_attrs.items(): setattr(record, key, value) return record
244-257: Clarify what the duplicate handler test is verifying.The test creates a grandchild logger (
child1→child2) but only checkschild2.handlers. Due to Python's logging hierarchy, this might not be testing what's intended. The test should clarify:
- Are you checking that
child2doesn't have duplicate handlers on itself?- Or that the handler isn't added redundantly when creating nested loggers?
Consider making the test more explicit:
# Create logger twice child1 = create_logger(parent_logger, "child", {"attr1": "value1"}) child2 = create_logger(child1, "grandchild", {"attr2": "value2"}) - # Should only have one MultiDelegateHandler + # child1 should have exactly one MultiDelegateHandler + multi_handlers_child1 = [h for h in child1.handlers if isinstance(h, MultiDelegateHandler)] + assert len(multi_handlers_child1) == 1 + + # child2 should also have exactly one (not duplicate when nested) multi_handlers = [h for h in child2.handlers if isinstance(h, MultiDelegateHandler)] assert len(multi_handlers) == 1
259-282: Test doesn't verify attributes in actual logging integration.The test manually applies filters to a LogRecord, simulating what the logging framework does. However, this doesn't test the actual integration—whether attributes correctly flow through when a real log message is emitted via the logger.
Consider adding an integration test that actually logs and verifies the full flow:
def test_create_logger_attributes_in_actual_logging(self): """Test that attributes are added when actually logging.""" mock_otel_logger = MagicMock() mock_provider = MagicMock() mock_provider.get_logger.return_value = mock_otel_logger add_user_logger_provider(mock_provider) parent_logger = logging.getLogger("test_parent") child = create_logger(parent_logger, "child", {"custom_attr": "test_value"}) # Actually log a message child.info("Test message") # Verify the custom attribute was passed through to OTEL mock_otel_logger.emit.assert_called() call_kwargs = mock_otel_logger.emit.call_args.kwargs assert 'custom_attr' in call_kwargs['attributes'] assert call_kwargs['attributes']['custom_attr'] == 'test_value'
1-282: Consider adding tests for timestamp conversion and message formatting.The test coverage is good for the main flows, but some details are untested:
- Timestamp conversion (line 44 in
emit_to_user_providers): No test verifies thatrecord.createdis correctly converted to nanoseconds- Message body: No test verifies the
bodyparameter passed toemit()matchesrecord.getMessage()- End-to-end integration: No test actually calls
logger.info(),logger.error(), etc., and verifies the complete flow through to user providersConsider adding tests like:
def test_emit_timestamp_conversion(self): """Verify timestamp is converted to nanoseconds.""" mock_otel_logger = MagicMock() mock_provider = MagicMock() mock_provider.get_logger.return_value = mock_otel_logger add_user_logger_provider(mock_provider) record = create_test_log_record() record.created = 1234567890.123456 # seconds with microseconds emit_to_user_providers(record) call_kwargs = mock_otel_logger.emit.call_args.kwargs expected_ns = int(1234567890.123456 * 1_000_000_000) assert call_kwargs['timestamp'] == expected_ns def test_integration_actual_logging(self): """Test complete flow with actual logging calls.""" # Set up user provider mock_otel_logger = MagicMock() mock_provider = MagicMock() mock_provider.get_logger.return_value = mock_otel_logger add_user_logger_provider(mock_provider) # Create logger and log message logger = logging.getLogger("integration_test") child = create_logger(logger, "test", {"session_id": "abc123"}) child.warning("Integration test message") # Verify OTEL emission happened with correct parameters mock_otel_logger.emit.assert_called_once() call_kwargs = mock_otel_logger.emit.call_args.kwargs assert call_kwargs['body'] == "Integration test message" assert 'session_id' in call_kwargs['attributes'] assert call_kwargs['attributes']['session_id'] == "abc123"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
agentuity/otel/__init__.py(4 hunks)agentuity/otel/logger.py(2 hunks)agentuity/server/__init__.py(2 hunks)pyproject.toml(1 hunks)tests/otel/test_user_logger.py(1 hunks)tests/otel/test_user_otel.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
agentuity/server/__init__.py (1)
agentuity/otel/__init__.py (2)
init(113-214)shutdown(217-232)
agentuity/otel/__init__.py (1)
agentuity/otel/logger.py (1)
add_user_logger_provider(9-13)
tests/otel/test_user_otel.py (1)
agentuity/otel/__init__.py (4)
parse_user_otel_config(28-56)UserOpenTelemetryConfig(17-25)create_user_logger_provider(63-110)shutdown(217-232)
tests/otel/test_user_logger.py (1)
agentuity/otel/logger.py (6)
add_user_logger_provider(9-13)emit_to_user_providers(16-62)MultiDelegateHandler(65-70)create_logger(73-104)emit(68-70)filter(90-94)
🔇 Additional comments (2)
agentuity/server/__init__.py (2)
17-17: Import rename LGTM.Clear alias for shutdown; no issues.
751-762: Graceful shutdown wiring LGTM.on_shutdown callback is correct; errors are contained; keeps server resilient.
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)
163-201: Critical: resource_attributes may be undefined when Traceloop import failsresource_attributes is created inside the try block but used later (Lines 209-211). If the import of traceloop fails before assignment, init will raise UnboundLocalError. Define resource_attributes outside the try and reuse it for Traceloop.init.
@@ - # Initialize traceloop for automatic instrumentation - try: - from traceloop.sdk import Traceloop - - headers = {"Authorization": f"Bearer {bearer_token}"} if bearer_token else {} - - resource_attributes = { - SERVICE_NAME: config.get( - "service_name", - app_name, - ), - SERVICE_VERSION: config.get( - "service_version", - app_version, - ), - "@agentuity/orgId": orgId, - "@agentuity/projectId": projectId, - "@agentuity/deploymentId": deploymentId, - "@agentuity/env": environment, - "@agentuity/devmode": devmode, - "@agentuity/sdkVersion": sdkVersion, - "@agentuity/cliVersion": cliVersion, - "@agentuity/language": "python", - "env": "dev" if devmode else "production", - "version": __version__, - } + # Build base resource attributes once; use for Traceloop and user OTEL + resource_attributes = { + SERVICE_NAME: config.get("service_name", app_name), + SERVICE_VERSION: config.get("service_version", app_version), + "@agentuity/orgId": orgId, + "@agentuity/projectId": projectId, + "@agentuity/deploymentId": deploymentId, + "@agentuity/env": environment, + "@agentuity/devmode": devmode, + "@agentuity/sdkVersion": sdkVersion, + "@agentuity/cliVersion": cliVersion, + "@agentuity/language": "python", + "env": "dev" if devmode else "production", + "version": __version__, + } + + # Initialize traceloop for automatic instrumentation + try: + from traceloop.sdk import Traceloop + headers = {"Authorization": f"Bearer {bearer_token}"} if bearer_token else {} @@ Traceloop.init( app_name=app_name, api_endpoint=endpoint, headers=headers, disable_batch=devmode, resource_attributes=resource_attributes, telemetry_enabled=False )Also applies to: 209-211
♻️ Duplicate comments (1)
agentuity/otel/__init__.py (1)
225-231: Shutdown order: flush processor/exporter before providerCall force_flush/shutdown on the processor and exporter before the provider to avoid tearing down the pipeline prematurely.
- # Shutdown components in reverse order: provider, processor, exporter - components = [ - ("provider", _user_logger_provider.get("provider")), - ("processor", _user_logger_provider.get("processor")), - ("exporter", _user_logger_provider.get("exporter")) - ] + # Flush and shutdown components in safe order: processor -> exporter -> provider + components = [ + ("processor", _user_logger_provider.get("processor")), + ("exporter", _user_logger_provider.get("exporter")), + ("provider", _user_logger_provider.get("provider")), + ]Also applies to: 236-258
🧹 Nitpick comments (3)
agentuity/otel/__init__.py (3)
79-86: Normalize endpoint when user supplies full OTLP pathIf endpoint already ends with /v1/logs (or /v1), appending /v1/logs creates invalid URLs. Normalize to accept both base and full paths.
- base_url = user_config.endpoint.rstrip('/') - logs_url = f"{base_url}/v1/logs" + base_url = user_config.endpoint.rstrip("/") + if base_url.endswith("/v1/logs"): + logs_url = base_url + elif base_url.endswith("/v1"): + logs_url = f"{base_url}/logs" + else: + logs_url = f"{base_url}/v1/logs"
119-121: Robust truthy parsing for AGENTUITY_OTLP_DISABLEDAccept common truthy forms (TRUE, True, 1, yes).
- if os.environ.get("AGENTUITY_OTLP_DISABLED", "false") == "true": + if os.environ.get("AGENTUITY_OTLP_DISABLED", "false").strip().lower() in {"true", "1", "yes"}:
28-50: Config parsing niceties: aliases and type guardsMinor DX polish: accept service_name/resource_attributes aliases and guard dict types.
- if not config_data.get("serviceName"): + service_name = config_data.get("serviceName") or config_data.get("service_name") + if not service_name: logger.warning("User OTEL config missing required 'serviceName' field, ignoring") return None @@ - return UserOpenTelemetryConfig( - endpoint=config_data["endpoint"], - service_name=config_data["serviceName"], - resource_attributes=config_data.get("resourceAttributes", {}), - headers=config_data.get("headers", {}) - ) + ra = config_data.get("resourceAttributes") or config_data.get("resource_attributes") or {} + if ra is not None and not isinstance(ra, dict): + logger.warning("User OTEL config 'resourceAttributes' must be an object; ignoring value") + ra = {} + hdrs = config_data.get("headers") or {} + if hdrs is not None and not isinstance(hdrs, dict): + logger.warning("User OTEL config 'headers' must be an object; ignoring value") + hdrs = {} + return UserOpenTelemetryConfig( + endpoint=config_data["endpoint"], + service_name=service_name, + resource_attributes=ra, + headers=hdrs, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
agentuity/otel/__init__.py(4 hunks)tests/otel/test_user_otel.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/otel/test_user_otel.py
🧰 Additional context used
🧬 Code graph analysis (1)
agentuity/otel/__init__.py (1)
agentuity/otel/logger.py (1)
add_user_logger_provider(13-24)
🔇 Additional comments (3)
agentuity/otel/__init__.py (3)
16-26: LGTM on UserOpenTelemetryConfigDataclass shape is clear and minimal; sensible defaults.
205-216: Remove the suggested refactor—no code paths call init multiple timesVerification found init() is called exactly once at agentuity/server/init.py:705 during server startup. No re-initialization paths exist in the codebase, so the defensive shutdown/idempotency logic is unnecessary.
63-91: Remove the suggested import path change; current private import is correct for 1.31.xThe public import path
opentelemetry.exporter.otlp.proto.http.log_exporterdoes not exist in OpenTelemetry Python 1.31.x. OTLPLogExporter is only provided under the private module path (opentelemetry.exporter.otlp.proto.http._log_exporter). The current code is correct and requires no changes.The optional gzip compression suggestion remains valid if needed for optimization.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/otel/test_user_logger.py (1)
119-166: Previous review feedback addressed; consider explicit attribute setting for clarity.The test now correctly verifies severity_number mapping (lines 161-164), addressing the prior review comment. However, the LogRecord created in lines 143-151 doesn't explicitly set
moduleandfuncName, whichemit_to_user_providersaccesses. While Python'sLogRecordinitializes these frompathname, explicitly setting them (as done intest_emit_to_user_providers_successlines 79-80) would improve test clarity and robustness.Apply this diff to explicitly set missing attributes:
record = logging.LogRecord( name="test", level=log_level, pathname="test.py", lineno=1, msg="Test message", args=(), exc_info=None ) + record.module = "test" + record.funcName = "test_func" # Call emit_to_user_providers emit_to_user_providers(record)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/otel/test_user_logger.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/otel/test_user_logger.py (1)
agentuity/otel/logger.py (6)
add_user_logger_provider(13-24)emit_to_user_providers(27-73)MultiDelegateHandler(76-83)create_logger(86-120)emit(79-83)filter(103-107)
🔇 Additional comments (2)
tests/otel/test_user_logger.py (2)
167-209: LGTM! Previous review feedback fully addressed.The test now correctly verifies that custom attributes are included (lines 202-205) and private attributes are filtered out (lines 207-208), addressing the prior review comment. The implementation is clear and thorough.
211-261: LGTM!The
TestMultiDelegateHandlerclass correctly validates both the active path (delegates toemit_to_user_providerswhen providers exist) and the no-op path (does nothing when no providers are registered). The tests are clear and comprehensive.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/otel/test_user_logger.py (1)
362-388: Test still doesn't properly verify retroactive handler attachment.The test clears both
child.handlersand_logger_registry(line 376), then creates a new loggerchild2(line 379). Whenadd_user_logger_provideris called (line 384), the retroactive logic cannot attach a handler tochildbecause it was removed from the registry. Meanwhile,child2gets a handler immediately fromcreate_logger, so there's nothing retroactive to test.To properly test retroactive attachment, the test should:
- Create a logger that's in the registry but lacks a handler
- Add a provider (triggering retroactive attachment)
- Verify the handler was retroactively added to that logger
Based on learnings
Apply this diff to fix the test:
def test_retroactive_handler_attachment(self): """Test that handlers are retroactively attached to existing loggers when providers are added.""" + from agentuity.otel.logger import _logger_registry + parent_logger = logging.getLogger("test_retroactive") # Create logger before adding any providers child = create_logger(parent_logger, "child", {"attr1": "value1"}) # Should have one handler (the MultiDelegateHandler) assert len(child.handlers) == 1 assert isinstance(child.handlers[0], MultiDelegateHandler) - # Clear handler and logger registry, then recreate logger without providers + # Remove handler to simulate logger in old state (before provider was added) child.handlers.clear() - from agentuity.otel.logger import _logger_registry - _logger_registry.clear() + assert len(child.handlers) == 0 + # child should still be in _logger_registry at this point + assert child in _logger_registry - # Create logger again without any providers - child2 = create_logger(parent_logger, "child2", {"attr2": "value2"}) - assert len(child2.handlers) == 1 # Should still get handler - - # Now add a provider - should not add duplicate handlers + # Add provider - should trigger retroactive attachment to child mock_provider = MagicMock() add_user_logger_provider(mock_provider) - # Should still have only one handler (no duplicates) - assert len(child2.handlers) == 1 - assert isinstance(child2.handlers[0], MultiDelegateHandler) + # Verify handler was retroactively attached to child + assert len(child.handlers) == 1 + assert isinstance(child.handlers[0], MultiDelegateHandler)
🧹 Nitpick comments (1)
tests/otel/test_user_logger.py (1)
119-166: Past review issue resolved, but consider adding missing attributes for consistency.The severity mapping verification has been properly fixed and now correctly asserts that the expected
severity_numberis passed tootel_logger.emit()for each logging level.However, for consistency with other tests (e.g., lines 79-80), consider setting
moduleandfuncNameon the LogRecord:for log_level, expected_severity in severity_mapping.items(): # Reset mock calls for clean test mock_otel_logger.reset_mock() mock_provider.reset_mock() record = logging.LogRecord( name="test", level=log_level, pathname="test.py", lineno=1, msg="Test message", args=(), exc_info=None ) + record.module = "test" + record.funcName = "test_function" # Call emit_to_user_providers emit_to_user_providers(record)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/otel/test_user_logger.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/otel/test_user_logger.py (1)
agentuity/otel/logger.py (6)
add_user_logger_provider(13-24)emit_to_user_providers(27-73)MultiDelegateHandler(76-83)create_logger(86-120)emit(79-83)filter(103-107)
🔇 Additional comments (7)
tests/otel/test_user_logger.py (7)
12-44: LGTM! Clean test setup and basic provider management.The setup/teardown pattern properly isolates tests by clearing the provider registry, and the basic provider addition tests are correct and straightforward.
46-117: LGTM! Emission tests cover key scenarios.The tests properly cover the no-provider path, successful emission, and error handling with appropriate verification of mocked calls and warning logs.
167-209: LGTM! Past review issue resolved.The test now properly verifies that custom attributes are included in the emitted attributes and that private attributes (those starting with
_) are correctly filtered out.
211-261: LGTM! Handler tests correctly verify conditional behavior.The tests properly verify that
MultiDelegateHandlerdelegates toemit_to_user_providerswhen providers exist and no-ops when no providers are registered.
263-299: LGTM! Basic create_logger tests are comprehensive.The tests correctly verify that
create_loggeradds aMultiDelegateHandlerin both scenarios (with and without user providers), with proper assertions on handler count, type, and level.
300-336: LGTM! Past review issue resolved.The test has been properly updated and now correctly verifies duplicate handler prevention. It simulates a scenario where a handler already exists and confirms that calling
create_loggeragain doesn't add another handler (stays at 2, not 3).
337-361: LGTM! Attributes filter test is well-designed.The test properly verifies that the
ContextFiltercreated bycreate_loggercorrectly adds custom attributes to log records.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/otel/test_user_logger.py (2)
119-166: Consider setting module and funcName explicitly for consistency.In this test,
LogRecordis created without explicitly settingmoduleandfuncName(line 143-151). While Python derivesmodulefrompathname,funcNamedefaults toNone. For consistency with other tests (e.g., lines 79-80) and clearer test intent, explicitly set these attributes.Apply this diff to improve consistency:
for log_level, expected_severity in severity_mapping.items(): # Reset mock calls for clean test mock_otel_logger.reset_mock() mock_provider.reset_mock() record = logging.LogRecord( name="test", level=log_level, pathname="test.py", lineno=1, msg="Test message", args=(), exc_info=None ) + record.module = "test" + record.funcName = "test_func" # Call emit_to_user_providers emit_to_user_providers(record)
167-209: Consider setting module and funcName explicitly for consistency.Similar to the severity mapping test, this test creates a
LogRecordwithout explicitly settingmoduleandfuncName. While functional, explicitly setting these attributes improves consistency and test clarity.Apply this diff:
record = logging.LogRecord( name="test", level=logging.INFO, pathname="test.py", lineno=1, msg="Test message", args=(), exc_info=None ) + record.module = "test" + record.funcName = "test_func" # Add custom attributes record.custom_attr = "custom_value"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/otel/test_user_logger.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/otel/test_user_logger.py (1)
agentuity/otel/logger.py (6)
add_user_logger_provider(13-24)emit_to_user_providers(27-73)MultiDelegateHandler(76-83)create_logger(86-120)emit(79-83)filter(103-107)
🔇 Additional comments (4)
tests/otel/test_user_logger.py (4)
1-21: LGTM! Proper test isolation with setup/teardown.The imports are appropriate, and the setup/teardown methods correctly clear
_user_logger_providersto ensure test isolation.
211-261: LGTM! Comprehensive coverage of MultiDelegateHandler behavior.The tests correctly verify both scenarios:
- Handler delegates to
emit_to_user_providerswhen providers exist- Handler no-ops when no providers exist
300-336: LGTM! Duplicate prevention logic correctly tested.The test properly verifies that
create_loggerdoesn't add a duplicateMultiDelegateHandlerwhen one already exists:
- Creates logger (1 handler)
- Manually adds another handler to simulate existing handlers (2 handlers)
- Calls
create_loggeragain and verifies it doesn't add a 3rd handler (still 2)The logic correctly addresses the previous review feedback.
362-384: LGTM! Retroactive attachment correctly tested.The test properly verifies retroactive handler attachment:
- Creates logger with handler via
create_logger(registers in_logger_registry)- Removes handler to simulate a logger in old state
- Adds provider, triggering retroactive attachment logic in
add_user_logger_provider- Verifies handler was retroactively attached
The test correctly addresses the previous review feedback.
This implementation mirrors the functionality from the JavaScript SDK PR #176, allowing users to send agent logs to their own OTLP endpoints alongside the default Agentuity telemetry collection.
Amp-Thread-ID: https://ampcode.com/threads/T-1dd20dcd-70fb-4f19-9de2-1984bb3b9a05
Summary by CodeRabbit
New Features
Tests
Chores