enable otlp metrics by default #172
Conversation
WalkthroughStartup now passes ApiVersion and the current environment name into OpenTelemetry setup. The monitoring extension accepts service version and environment, builds an OpenTelemetry Resource with serviceName/serviceVersion and adds an "environment" attribute, then continues registering instrumentation and OTLP exporter as before. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application Startup
participant Bootstrap as Bootstrap.cs
participant Monitoring as MonitoringExtension
participant OTEL as OpenTelemetry SDK
participant Collector as OTLP Collector
App->>Bootstrap: start, obtain ApiVersion and CurrentEnvironment.EnvironmentName
Bootstrap->>Monitoring: AddLoggingAndMetricTracing(serviceName, serviceVersion, environmentName)
Monitoring->>OTEL: Build Resource (serviceName, serviceVersion) and Add "environment" attribute
Monitoring->>OTEL: Register instrumentations (ASP.NET Core, HttpClient, Runtime) and OTLP exporter
OTEL->>Collector: Export traces/metrics via OTLP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@src/Simplic.OxS.Server/Bootstrap.cs`:
- Around line 161-181: The AddOpenTelemetry() block in Bootstrap.cs is
duplicating metrics/instrumentation already registered by
AddLoggingAndMetricTracing(); remove the duplicate AddOpenTelemetry() call and
its chained ConfigureResource/WithMetrics configuration from Bootstrap.cs, or
alternatively move any unique configuration (e.g., resource attributes like
serviceName/serviceVersion/serviceInstanceId or adding the "environment"
attribute) into the existing AddLoggingAndMetricTracing() /
MonitoringExtension.cs implementation so instrumentation
(AddAspNetCoreInstrumentation, AddHttpClientInstrumentation,
AddRuntimeInstrumentation, AddOtlpExporter) is only registered once.
🧹 Nitpick comments (3)
src/Simplic.OxS.Server/Bootstrap.cs (3)
157-158: Use the injectedCurrentEnvironmentinstead of re-reading the environment variable.
CurrentEnvironment.EnvironmentNameis already available and used at line 54. This avoids redundant environment variable access and maintains consistency.Suggested change
- var stage = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") ?? "Production"; - if (stage == "Staging") + if (CurrentEnvironment.IsStaging())If you still need the
stagevariable for the attribute:- var stage = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") ?? "Production"; + var stage = CurrentEnvironment.EnvironmentName; if (stage == "Staging")
160-166: Consider handling emptyServiceNamedefensively.
ServiceNameis abstract so it won't be null, but it could be an empty string. The null-coalescing operator won't catch that case.Suggested change
- var appName = ServiceName ?? "unset"; + var appName = string.IsNullOrWhiteSpace(ServiceName) ? "unset" : ServiceName;
168-171: Hardcoded "environment" attribute value doesn't match the variable.The
stagevariable is used in the condition check, but the attribute value is hardcoded as"Staging". While functionally equivalent in this context (since it's inside theif (stage == "Staging")block), using the variable would be more maintainable if this pattern expands to other environments.Suggested change
// Add stage to ALL metrics r.AddAttributes([ - new KeyValuePair<string, object>("environment", stage) + new KeyValuePair<string, object>("environment", CurrentEnvironment.EnvironmentName) ]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Simplic.OxS.Server/Bootstrap.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-26T05:52:00.195Z
Learnt from: CR
Repo: simplic/simplic-oxs-storage-management PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-26T05:52:00.195Z
Learning: Applies to {Program.cs,**/Middleware/**/*.cs,**/Logging/**/*.cs,**/Telemetry/**/*.cs} : Use structured logging (e.g., Serilog), appropriate log levels, Application Insights integration, custom telemetry and correlation IDs; monitor performance, errors, usage
Applied to files:
src/Simplic.OxS.Server/Bootstrap.cs
📚 Learning: 2025-11-26T09:38:34.885Z
Learnt from: CR
Repo: simplic/simplic-oxs-accounting PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-26T09:38:34.885Z
Learning: Use built-in ASP.NET Core features when possible before introducing third-party dependencies
Applied to files:
src/Simplic.OxS.Server/Bootstrap.cs
🔇 Additional comments (2)
src/Simplic.OxS.Server/Bootstrap.cs (2)
15-16: LGTM on imports.The OpenTelemetry imports are appropriate for the metrics configuration being added.
156-182: PR title says "enable otlp metrics by default" but metrics are only enabled for Staging.The implementation restricts OpenTelemetry metrics to only the
Stagingenvironment. If this is intentional (gradual rollout), consider updating the PR description to clarify. If metrics should be enabled more broadly, the condition logic needs adjustment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Simplic.OxS.Server/Extensions/MonitoringExtension.cs`:
- Around line 39-56: The ConfigureResource block is missing the serviceVersion,
causing tracing/metrics to lack it; update the ConfigureResource call inside
AddOpenTelemetry to include the serviceVersion in its AddService invocation
(e.g., call AddService(serviceName: $"oxs-{serviceName}", serviceVersion:
serviceVersion) or pass the previously built resourceBuilder into
AddOpenTelemetry) so that resourceBuilder and ConfigureResource are consistent
and tracing/metrics include the same serviceVersion as logging.
♻️ Duplicate comments (1)
src/Simplic.OxS.Server/Bootstrap.cs (1)
157-182: Remove duplicate OpenTelemetry configuration; this block duplicates instrumentation already registered byAddLoggingAndMetricTracing().As noted in a previous review,
AddLoggingAndMetricTracing()(line 63) already configures OpenTelemetry with the same metrics instrumentation (AddAspNetCoreInstrumentation,AddHttpClientInstrumentation,AddRuntimeInstrumentation) inMonitoringExtension.cs. This secondAddOpenTelemetry()call creates duplicate metric collection and export.Additionally, line 157 redundantly reads
ASPNETCORE_ENVIRONMENTwhenCurrentEnvironment.EnvironmentNameis already available and passed toAddLoggingAndMetricTracing().If Staging-specific behavior is needed (e.g., always enabling OTLP export), integrate that logic into
MonitoringExtension.csrather than duplicating the entire OpenTelemetry setup.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Simplic.OxS.Server/Bootstrap.cssrc/Simplic.OxS.Server/Extensions/MonitoringExtension.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-26T05:52:00.195Z
Learnt from: CR
Repo: simplic/simplic-oxs-storage-management PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-26T05:52:00.195Z
Learning: Applies to {Program.cs,**/Middleware/**/*.cs,**/Logging/**/*.cs,**/Telemetry/**/*.cs} : Use structured logging (e.g., Serilog), appropriate log levels, Application Insights integration, custom telemetry and correlation IDs; monitor performance, errors, usage
Applied to files:
src/Simplic.OxS.Server/Extensions/MonitoringExtension.cssrc/Simplic.OxS.Server/Bootstrap.cs
📚 Learning: 2025-11-26T09:38:34.885Z
Learnt from: CR
Repo: simplic/simplic-oxs-accounting PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-26T09:38:34.885Z
Learning: Use built-in ASP.NET Core features when possible before introducing third-party dependencies
Applied to files:
src/Simplic.OxS.Server/Bootstrap.cs
🔇 Additional comments (3)
src/Simplic.OxS.Server/Bootstrap.cs (2)
15-16: LGTM!The OpenTelemetry namespace imports are correctly added to support the new metrics configuration.
61-63: LGTM!Correctly passes the
ApiVersionandCurrentEnvironment.EnvironmentNameto the updatedAddLoggingAndMetricTracingmethod signature, enabling environment-aware telemetry configuration.src/Simplic.OxS.Server/Extensions/MonitoringExtension.cs (1)
27-27: LGTM!The updated method signature appropriately accepts
serviceVersionandenvironmentNameparameters to enable version-aware and environment-aware telemetry configuration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.