add support for OTel log emission #435
Conversation
WalkthroughThe pull request introduces OpenTelemetry (OTEL) logging integration by adding configuration flags, a new OTEL logger provider module, log sink forwarding logic, and initialization wiring in the voice agent system. Configuration endpoints and enablement flags are exposed for environment-based control. Changes
Sequence DiagramsequenceDiagram
participant Agent as Voice Agent
participant Config as Configuration
participant OtelSetup as OTEL Setup Module
participant LoggerInit as Logger Module
participant Provider as OTEL Provider
participant Crane as Crane Endpoint
Agent->>Config: Read ENABLE_OTEL_LOGS & OTEL_EXPORTER_OTLP_LOGS_ENDPOINT
Agent->>OtelSetup: Call setup_otel_logging(service_name)
OtelSetup->>OtelSetup: Check _otel_logging_initialized flag
OtelSetup->>Provider: Create Resource & LoggerProvider
OtelSetup->>Provider: Add BatchLogRecordProcessor & OTLPLogExporter
OtelSetup->>LoggerInit: Call set_otel_logger_provider(provider)
LoggerInit->>LoggerInit: Store global _otel_logger_provider reference
OtelSetup->>LoggerInit: Add otel_sink to Loguru
LoggerInit->>Provider: Register sink for log forwarding
Note over LoggerInit,Provider: Subsequent logs flow through<br/>Loguru → otel_sink → OTEL Provider
LoggerInit->>Crane: Export logs via OTLP (batch)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 1
🧹 Nitpick comments (4)
app/core/logger/__init__.py (2)
87-94: Minor: Avoid duplicatetime.time()calls.The timestamp and observed_timestamp are computed with separate
time.time()calls, which could result in slightly different values. Capture the timestamp once for consistency and minor efficiency gains.🔎 Apply this diff:
+ now_ns = int(time.time() * 1e9) log_record = APILogRecord( - timestamp=int(time.time() * 1e9), # nanoseconds - observed_timestamp=int(time.time() * 1e9), + timestamp=now_ns, # nanoseconds + observed_timestamp=now_ns, body=record["message"], severity_number=severity, attributes=attributes, )
81-82: Consider parameterizing the logger name.The logger name
"clairvoyance"is hardcoded here, whilesetup_otel_loggingaccepts aservice_nameparameter. If the service name changes, this would become inconsistent. Consider storing the service name globally alongside the provider, or using a constant.app/core/logger/otel_logs.py (2)
79-84: Remove unused import.
LoggingHandleris imported but never used in this function.🔎 Apply this diff:
from opentelemetry._logs import set_logger_provider from opentelemetry.exporter.otlp.proto.http._log_exporter import OTLPLogExporter - from opentelemetry.sdk._logs import LoggerProvider, LoggingHandler + from opentelemetry.sdk._logs import LoggerProvider from opentelemetry.sdk._logs.export import BatchLogRecordProcessor from opentelemetry.sdk.resources import Resource
109-126: Consider atomic initialization to avoid partial setup state.If an exception occurs after
set_otel_logger_provider(logger_provider)(line 112) but before_otel_logging_initialized = True(line 125), the provider is set globally but the sink is not added. On retry, the idempotency guard would prevent completion. Consider setting_otel_logging_initialized = Trueearlier or restructuring to ensure atomicity.🔎 One approach - set the flag at the end of try block, after all operations succeed:
The current structure is actually correct since the flag is set after all operations complete successfully. However, if
logger.add()fails, the provider is already set globally which could lead to inconsistent state. Consider wrapping the provider setup in a way that can be rolled back:# Set the global OTEL logger provider in __init__.py so otel_sink can use it from app.core.logger import set_otel_logger_provider - set_otel_logger_provider(logger_provider) - # Add OTEL sink to Loguru from app.core.logger import otel_sink logger.add( otel_sink, level="DEBUG", # Capture all levels, filtering happens in the sink enqueue=True, # Thread-safe async logging backtrace=False, diagnose=False, ) + # Set provider only after sink is successfully added + set_otel_logger_provider(logger_provider) + _otel_logging_initialized = True
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/ai/voice/agents/automatic/__init__.py(2 hunks)app/core/config/static.py(1 hunks)app/core/logger/__init__.py(1 hunks)app/core/logger/otel_logs.py(1 hunks)run.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/ai/voice/agents/automatic/__init__.py (1)
app/core/logger/otel_logs.py (1)
setup_otel_logging(31-131)
🪛 Ruff (0.14.8)
app/core/logger/otel_logs.py
130-130: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
app/ai/voice/agents/automatic/__init__.py (2)
96-96: LGTM!Import is correctly placed alongside other analytics-related imports.
614-617: LGTM!The OTEL logging initialization is appropriately placed after session context setup and before tracing setup. The
setup_otel_loggingfunction has an internal idempotency guard, so this is safe to call per session in pool mode.app/core/config/static.py (1)
134-137: Verify the default endpoint is appropriate for production.The default endpoint
https://crane.beta.breeze.inpoints to a beta environment. WhenENABLE_OTEL_LOGSis enabled without explicitly settingOTEL_EXPORTER_OTLP_LOGS_ENDPOINT, logs will be sent to the beta environment. Consider whether:
- The default should be an empty string (like other similar configs), requiring explicit configuration
- Or the beta default is intentional for gradual rollout
app/core/logger/__init__.py (3)
35-46: LGTM!Good design with a global reference and early return when provider is not set. This ensures minimal overhead when OTEL logging is disabled.
48-59: LGTM!The filtering logic correctly prevents feedback loops by skipping logs that originate from OpenTelemetry internals or reference the Crane endpoint.
97-104: LGTM!Simple and clean setter function for the global provider.
app/core/logger/otel_logs.py (4)
1-28: LGTM!Clear module docstring with usage example, and appropriate severity level mapping for Loguru-to-OTEL translation.
49-68: LGTM!Good approach: the CraneLogFilter is applied regardless of whether OTEL logging is enabled, ensuring the standard library loggers are always filtered for Crane-related noise.
128-131: Catching broadExceptionis acceptable here.The static analysis flags the broad exception catch, but for a logging subsystem, it's appropriate to catch all exceptions to prevent logging initialization from crashing the application. The error is logged before returning, which is the correct behavior.
134-143: LGTM!Simple getter function that correctly imports and returns the global provider reference.
| asyncio.run(initialize_devcycle()) | ||
|
|
||
| # STEP 5: Start uvicorn server (will spawn worker processes) | ||
| # STEP 6: Start uvicorn server (will spawn worker processes) |
There was a problem hiding this comment.
Missing STEP 5 in the sequence.
The comment was changed to "STEP 6" but there's no "STEP 5" in the file. The sequence goes from STEP 4 (line 56) directly to STEP 6. This may confuse future maintainers. Either add the missing STEP 5 comment (perhaps for OTEL initialization if that was intended) or use STEP 5 here.
🤖 Prompt for AI Agents
In run.py around line 60, the inline step numbering jumps from "STEP 4" to "STEP
6"; add or correct the comment so sequence is contiguous. Replace "STEP 6" with
"STEP 5" (or insert a new "STEP 5" comment before this line if there is an
intended OTEL or other initialization step to document) to restore proper
sequential ordering and avoid confusion for future maintainers.
|
|
||
| import time | ||
|
|
||
| from app.core.logger.otel_logs import LEVEL_TO_SEVERITY |
2 participants
add support for OTel log emission
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.