Adds a new env var that enable multiple otel providers #985
Conversation
Tagging OptionsShould a new tag be published when this PR is merged?
|
WalkthroughThe changes introduce fan-out OTLP exporter support by adding environment-based configuration parsing and multi-exporter initialization. A new Changes
Sequence DiagramsequenceDiagram
participant Init as OpenTelemetry Initialization
participant Config as Config Parser
participant Logger as OpenTelemetryHandler
participant Provider as LoggerProvider
participant MainProc as Main Processor
participant ExtraProc as Extra Processors
Init->>Config: parseExtraOTLPExporters()
Config->>Config: Read OTEL_EXPORTER_EXTRA_OTLP_HANDLER
Config->>Config: Parse & validate JSON array
Config-->>Init: Return exporter configs (or [])
Init->>Logger: Create with additionalExporters
Logger->>Provider: Initialize LoggerProvider
Logger->>MainProc: Create main BatchLogRecordProcessor
Logger->>Provider: Add main processor
alt Extra exporters present
Logger->>ExtraProc: Create BatchLogRecordProcessor per exporter
ExtraProc->>Provider: Add each processor
Logger->>Logger: Store all in _processors[]
else No extra exporters
Logger->>Logger: _processors = [main]
end
Note over Logger,Provider: All processors fanout logs to respective exporters
Logger->>Logger: On destroy: shutdown all _processors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes The changes introduce multi-processor state management and environment-based configuration with validation logic. While the modifications span two related files and involve error handling, the refactoring follows a consistent pattern and doesn't introduce highly complex interdependencies. Review complexity is driven primarily by verifying correct processor lifecycle management and config parsing robustness. Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 (2)
observability/otel/logger.ts (1)
107-119: Guard extras when main exporter is console (avoid mixed mode).If
exporterProtocol === "console", adding HTTP OTLP extra exporters is surprising. Either ignore with a warning or disallow.Apply this diff:
- if (options.additionalExporters && options.additionalExporters.length > 0) { + if ( + options.exporterProtocol !== "console" && + options.additionalExporters && + options.additionalExporters.length > 0 + ) { options.additionalExporters.forEach((exporterConfig) => { const additionalExporter = new OTLPLogExporter(exporterConfig); const additionalProcessor = new BatchLogRecordProcessor( // @ts-ignore: no idea why this is failing, but it should work additionalExporter, options.processorConfig, ); this._processors.push(additionalProcessor); loggerProvider.addLogRecordProcessor(additionalProcessor); }); - } + } else if ( + options.exporterProtocol === "console" && + options.additionalExporters?.length + ) { + console.warn( + "additionalExporters provided but exporterProtocol=console; ignoring extras", + ); + }observability/otel/config.ts (1)
157-161: Add explicit return type, import OTLPExporterNodeConfigBase, and harden header validation.The review comment is valid. The current implementation lacks type safety for environment variable parsing and doesn't validate that headers contain only strings. The web search confirms that OTLPExporterNodeConfigBase accepts
{ url: string, headers: record }configuration, and the type is already exported from deps.ts (line 91) but not imported in config.ts.Apply the suggested changes:
- Add the missing type import
OTLPExporterNodeConfigBasefrom deps.ts- Add explicit return type annotation
OTLPExporterNodeConfigBase[]to the function- Change endpoint validation from truthiness check to explicit string type and length validation
- Sanitize headers to filter non-string values, preventing malformed config from reaching the OTLP exporter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
observability/otel/config.ts(2 hunks)observability/otel/logger.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
observability/otel/config.ts (1)
observability/otel/logger.ts (1)
OpenTelemetryHandler(66-174)
observability/otel/logger.ts (1)
deps.ts (7)
OTLPExporterNodeConfigBase(91-91)Logger(88-88)BatchLogRecordProcessor(100-100)LoggerProvider(102-102)Resource(63-63)ConsoleLogRecordExporter(101-101)OTLPLogExporter(89-89)
🔇 Additional comments (5)
observability/otel/config.ts (1)
61-69: LGTM on fan‑out wiring into logger.The handler integration with additionalExporters is correct and scoped under OTEL_IS_ENABLED.
observability/otel/logger.ts (4)
63-64: HandlerOptions extension looks good.
additionalExporters?: OTLPExporterNodeConfigBase[]is a clean, minimal surface change.
68-68: Storing processors array: good call.Allows proper teardown of all fan‑out processors.
88-93: Provider initialization and resource merge: OK.Creating the LoggerProvider early and merging detected + custom resource attributes is correct.
99-106: Main exporter/processor setup: OK.Batching around the selected exporter with passed processorConfig is standard.
| // Shutdown all processors | ||
| this._processors.forEach((processor) => processor.shutdown()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Best‑effort flush before shutdown.
shutdown() is async; in a sync destroy, at least trigger forceFlush() before shutdown for better delivery guarantees.
Apply this diff:
- // Shutdown all processors
- this._processors.forEach((processor) => processor.shutdown());
+ // Flush then shutdown all processors (best-effort; cannot await here)
+ this._processors.forEach((processor) => {
+ // @ts-ignore: ignore Promise return in sync context
+ processor.forceFlush();
+ // @ts-ignore: ignore Promise return in sync context
+ processor.shutdown();
+ });If this handler owns the global provider, consider also keeping a reference to loggerProvider and calling its shutdown() here. Verify there aren’t multiple owners:
🏁 Script executed:
#!/bin/bash
# Find other places setting a global LoggerProvider
rg -n -C2 'setGlobalLoggerProvider\s*\('Length of output: 298
I need to inspect the full class structure to understand variable scope and whether the suggestion is valid.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full context around the initialization and shutdown methods
cat observability/otel/logger.ts | head -180 | tail -80
Length of output: 2537
I need to see the class declaration and member variables to verify scope.
---
🏁 Script executed:
```shell
#!/bin/bash
# View the class declaration and member variables
cat observability/otel/logger.ts | head -120
Length of output: 3442
I need to verify OpenTelemetry shutdown best practices to validate the review's suggestion.
---
🌐 Web query:
```web
OpenTelemetry LoggerProvider shutdown method best practices
💡 Result:
Short best-practices checklist for LoggerProvider shutdown (OpenTelemetry):
- Call ForceFlush before Shutdown to try to export pending log records. Configure/observe a reasonable timeout. [1][2]
- Call Shutdown exactly once per LoggerProvider and per LogRecordProcessor/exporter; after shutdown loggers should become no-op. [1]
- Make Shutdown bounded (don’t block indefinitely). Prefer async Shutdown with a caller-side timeout you control (or a configurable SDK timeout). [1][2]
- Ensure Shutdown invokes Shutdown on all registered LogRecordProcessors and that Shutdown includes ForceFlush semantics. [1]
- Register a process/application-lifecycle hook to run ForceFlush → Shutdown on graceful termination; in short-lived/FaaS environments call ForceFlush immediately after work to avoid dropped logs. [1][2]
- Tune batching/exporter timeouts (export timeout, maxQueueSize, scheduledDelay, maxExportBatchSize) so Shutdown/ForceFlush can finish within your chosen timeout. [1]
- Handle and surface errors/timeouts from ForceFlush/Shutdown (log them, retry if appropriate) and prefer making the shutdown path resilient to exporter failures. [1][2][3]
- For language-specific frameworks (e.g., .NET), properly dispose/stop the LoggerFactory/Provider to avoid dropped logs. [3]
Sources: OpenTelemetry Logs SDK spec (Shutdown / ForceFlush), Tracing SDK guidance, and language-specific best-practices (e.g., .NET). [1][2][3]
References:
[1] OpenTelemetry Logs SDK — Shutdown & ForceFlush.
[2] OpenTelemetry Trace SDK — ForceFlush / Shutdown guidance.
[3] OpenTelemetry language docs (dotnet) — LoggerFactory lifecycle / dispose guidance.
Store LoggerProvider and ensure proper ForceFlush + Shutdown on both provider and processors.
OpenTelemetry best practices require calling ForceFlush before Shutdown on both the LoggerProvider and all LogRecordProcessors to ensure pending logs are exported. The current code only handles processors; the LoggerProvider itself must also be shut down.
The handler creates and sets the global LoggerProvider (line 121), making it responsible for its lifecycle. The suggested diff is incomplete—it adds forceFlush() before shutdown() on processors, but loggerProvider is not stored as a member, so it cannot be accessed in destroy().
Refactor to:
- Store LoggerProvider as a member variable:
protected _loggerProvider: LoggerProvider - Call both
forceFlush()andshutdown()on the provider indestroy() - Apply the same pattern to all processors (as shown in the original diff)
🤖 Prompt for AI Agents
In observability/otel/logger.ts around lines 170-171, the destroy() logic only
shuts down processors and doesn't store or shut down the LoggerProvider; update
the class to add a protected member _loggerProvider: LoggerProvider, assign
this._loggerProvider when creating/setting the global provider, and in destroy()
call await processor.forceFlush() then await processor.shutdown() for each
processor and also call await this._loggerProvider.forceFlush() followed by
await this._loggerProvider.shutdown(); ensure proper await/try-catch around
these calls and no-op if _loggerProvider is undefined.
Summary by CodeRabbit