Conversation
yordis
commented
May 3, 2026
- EventStore needs all OpenTelemetry signals to leave the node through the same OTLP configuration surface.
- Operators should not need a separate tracing setup model when logs and metrics already support shared and signal-specific OTLP destinations.
- Baseline request tracing makes the observability path complete enough for collector-backed deployments to validate logs, metrics, and traces together.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryMedium Risk Overview Wires tracing into node startup via OpenTelemetry Reviewed by Cursor Bugbot for commit 401fc38. Bugbot is set up for automated code reviews on this repo. Configure here. |
WalkthroughThis pull request adds OpenTelemetry traces support to EventStore's OTLP export infrastructure. Changes include: new trace configuration prefixes and enablement check; tracing setup in ClusterVNodeStartup with ASP.NET Core instrumentation; the OpenTelemetry.Instrumentation.AspNetCore NuGet dependency; comprehensive test coverage of OTLP traces configuration behavior; and updated documentation reflecting traces alongside existing metrics and logs exports. ChangesOpenTelemetry Traces Support
Sequence DiagramsequenceDiagram
participant App as Application<br/>Startup
participant Config as Configuration<br/>System
participant OTLP as OpenTelemetry<br/>Builder
participant Tracer as Tracer<br/>Provider
participant Instr as ASP.NET Core<br/>Instrumentation
participant Exporter as OTLP<br/>Exporter
App->>Config: Read EventStore:OpenTelemetry:Traces config
Config-->>App: Traces enabled + OTLP endpoint
App->>OTLP: ConfigureTracing(builder, config)
OTLP->>Tracer: Set service resource name = "eventstore"
OTLP->>Instr: Add AspNetCore instrumentation
OTLP->>Exporter: Configure OTLP exporter with endpoint
Exporter-->>OTLP: Exporter ready
Note over App,Exporter: At request time
App->>Instr: HTTP request arrives
Instr->>Tracer: Record span
Tracer->>Exporter: Export spans
Exporter-->>Exporter: Send to OTLP collector
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 401fc38. Configure here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/EventStore.Core/ClusterVNodeStartup.cs (1)
265-265: ⚡ Quick winExtract the
"eventstore"service-name literal into a shared constant.The service name
"eventstore"is used identically in bothConfigureMetrics(line 265) andConfigureTracing(line 347). Extracting it prevents a future rename from silently diverging the two signal pipelines.♻️ Proposed refactor
+ private const string ServiceName = "eventstore"; private static void ConfigureMetrics(...) { meterOptions - .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("eventstore")) + .SetResourceBuilder(ResourceBuilder.CreateDefault().AddService(ServiceName)) ... } private static void ConfigureTracing(...) { - tracerOptions.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService("eventstore")); + tracerOptions.SetResourceBuilder(ResourceBuilder.CreateDefault().AddService(ServiceName)); ... }Also applies to: 347-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventStore.Core/ClusterVNodeStartup.cs` at line 265, Extract the literal "eventstore" into a shared constant and use that constant in both metrics and tracing setup to avoid divergence: define a const string (e.g., ServiceName or EventStoreServiceName) and replace occurrences of ResourceBuilder.CreateDefault().AddService("eventstore") and the matching call in ConfigureTracing with ResourceBuilder.CreateDefault().AddService(ServiceName) (or the chosen constant) so both ConfigureMetrics and ConfigureTracing reference the same identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/EventStore.Core/ClusterVNodeStartup.cs`:
- Line 265: Extract the literal "eventstore" into a shared constant and use that
constant in both metrics and tracing setup to avoid divergence: define a const
string (e.g., ServiceName or EventStoreServiceName) and replace occurrences of
ResourceBuilder.CreateDefault().AddService("eventstore") and the matching call
in ConfigureTracing with ResourceBuilder.CreateDefault().AddService(ServiceName)
(or the chosen constant) so both ConfigureMetrics and ConfigureTracing reference
the same identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c961157c-126a-4a3d-a794-bccb94fc7f29
📒 Files selected for processing (8)
docs/diagnostics/integrations.mdqodana.yamlsrc/Directory.Packages.propssrc/EventStore.Core.XUnit.Tests/OpenTelemetry/OtlpTracingConfigurationTests.cssrc/EventStore.Core/ClusterVNodeStartup.cssrc/EventStore.Core/Configuration/OpenTelemetryConfiguration.cssrc/EventStore.Core/EventStore.Core.csprojsrc/qodana.yaml
