feat(apps): export logs to open telemetry endpoint #1617
Conversation
📝 WalkthroughWalkthroughThis pull request introduces extensive changes to the project's observability and local development infrastructure. Key modifications include enhanced logging, tracing, and metrics collection through the integration of Loki for log aggregation and updates to the OpenTelemetry configuration. The Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
@MagnusSandgren pretty sure we don't need this anymore, or that there is more data that we need to enrich the logs with 🤔 |
![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 (10)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
191-195: Consider structured logging for unexpected exceptions.Currently, the application logs to the console for unexpected exceptions. Structured logging (e.g. JSON) can improve troubleshooting and correlation, especially if the logs are streamed to external services.
-catch (Exception ex) when (ex is not OperationCanceledException) -{ - Console.WriteLine($"Application terminated unexpectedly: {ex}"); - throw; -} +catch (Exception ex) when (ex is not OperationCanceledException) +{ + // Potential alternative approach + Log.Logger?.Fatal(ex, "Application terminated unexpectedly."); + throw; +}local-otel-configuration/grafana-datasources.yml (1)
8-15: Add newline at end of file.A trailing newline at the end of the file is recommended for better compatibility with various tools and text editors, as highlighted by yamllint.
maxLines: 1000 +🧰 Tools
🪛 yamllint (1.35.1)
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
src/Digdir.Domain.Dialogporten.WebApi/Common/Middleware/RequestLoggingMiddleware.cs (1)
17-32: Add filtering for health check and metrics endpointsTo reduce noise in logs, consider adding filtering for frequently called monitoring endpoints.
public async Task InvokeAsync(HttpContext context) { + // Skip logging for health checks and metrics + if (context.Request.Path.StartsWithSegments("/health") || + context.Request.Path.StartsWithSegments("/metrics")) + { + await _next(context); + return; + } try { await _next(context);src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (1)
14-14: Consider conditionally including the Console exporterThe Console exporter is typically used for development/debugging. Consider moving it to a development-only configuration to reduce overhead in production.
You could:
- Move it to a development-only package reference:
<ItemGroup Condition="'$(Configuration)' == 'Debug'"> <PackageReference Include="OpenTelemetry.Exporter.Console" Version="1.10.0" /> </ItemGroup>
- Or configure it conditionally in code using environment variables/configuration settings.
docker-compose-otel.yml (3)
40-54: Add volume persistence for Loki storageCurrently, Loki's data will be lost when the container is removed. Consider adding a named volume for data persistence.
loki: image: grafana/loki:3.2.2 ports: - "3100:3100" volumes: - ./local-otel-configuration/loki-config.yaml:/etc/loki/local-config.yaml + - loki-data:/loki command: -config.file=/etc/loki/local-config.yaml + volumes: + loki-data:
40-54: Add resource limits for LokiTo prevent resource exhaustion, consider adding memory and CPU limits for the Loki service.
loki: image: grafana/loki:3.2.2 + deploy: + resources: + limits: + memory: 1G + cpus: '1' + reservations: + memory: 256M + cpus: '0.5'
67-68: Enhance service dependency configurationThe Grafana dependency on Loki should include a health check condition to ensure Loki is fully ready.
depends_on: - - loki + loki: + condition: service_healthyREADME.md (3)
Line range hint
129-138: Consider adding security considerations for OpenTelemetry configuration.While the OpenTelemetry configuration is well documented, consider adding a note about securing sensitive information in environment variables, particularly:
- Connection string handling best practices
- Endpoint security considerations
- Resource attribute sanitization
161-226: Consider adding troubleshooting guidance for local development.The local development setup is well documented, but consider adding:
- Port conflict resolution steps
- Common troubleshooting scenarios
- Health check verification steps
- Resource requirements and limitations
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used(MD034, no-bare-urls)
185-185: null
Bare URL used(MD034, no-bare-urls)
192-192: null
Bare URL used(MD034, no-bare-urls)
193-193: null
Bare URL used(MD034, no-bare-urls)
208-208: null
Bare URL used(MD034, no-bare-urls)
177-177: Format URLs according to markdown best practices.Convert bare URLs to proper markdown link format for better documentation quality:
-http://localhost:16686 +[http://localhost:16686](http://localhost:16686) -http://localhost:9090 +[http://localhost:9090](http://localhost:9090) -http://localhost:3100 +[http://localhost:3100](http://localhost:3100) -http://localhost:3000 +[http://localhost:3000](http://localhost:3000)Also applies to: 185-185, 192-192, 193-193, 208-208
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used(MD034, no-bare-urls)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(2 hunks)docker-compose-otel.yml(3 hunks)local-otel-configuration/grafana-datasources.yml(1 hunks)local-otel-configuration/loki-config.yaml(1 hunks)local-otel-configuration/otel-collector-config.yaml(2 hunks)src/Digdir.Domain.Dialogporten.WebApi.Tests/Common/Middleware/RequestLoggingMiddlewareTests.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Middleware/RequestLoggingMiddleware.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(3 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Digdir.Domain.Dialogporten.WebApi.Tests/Common/Middleware/RequestLoggingMiddlewareTests.cs
🧰 Additional context used
🪛 yamllint (1.35.1)
local-otel-configuration/grafana-datasources.yml
[error] 15-15: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Markdownlint (0.37.0)
README.md
177-177: null
Bare URL used
(MD034, no-bare-urls)
185-185: null
Bare URL used
(MD034, no-bare-urls)
192-192: null
Bare URL used
(MD034, no-bare-urls)
193-193: null
Bare URL used
(MD034, no-bare-urls)
208-208: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (10)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (2)
17-18: LGTM: Required imports for OpenTelemetry logging integration.
The added imports are necessary for implementing OpenTelemetry logging functionality.
162-168: 🛠️ Refactor suggestion
Consider adding additional logging configurations.
While the basic OpenTelemetry logging setup is good, consider adding the following configurations to align with the PR objectives:
- Log enrichment for additional metadata
- Exception handling configuration, especially for Redis exceptions
- Log level configuration
- Sampling configuration for production environments
Here's a suggested enhancement:
if (!builder.Environment.IsDevelopment())
{
// Clear existing logging providers. If development, we want to keep the console logging.
builder.Logging.ClearProviders();
}
-telemetryBuilder.WithLogging();
+telemetryBuilder.WithLogging(logging =>
+{
+ // Configure log levels
+ logging.SetMinimumLevel(LogLevel.Information);
+
+ // Add enrichment
+ logging.AddProcessor(new CustomEnrichmentProcessor());
+
+ // Configure sampling in production
+ if (!builder.Environment.IsDevelopment())
+ {
+ logging.AddSampler(new TraceIdRatioBasedSampler(0.1));
+ }
+
+ // Configure exception handling
+ logging.AddExceptionProcessor(new CustomExceptionProcessor());
+});Let's verify if there are any existing logging configurations in the codebase:
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3)
29-29: Streamlined startup approach looks good.
Switching to the simplified WebApplication builder pattern is a clean approach that reduces complexity in your initialization sequence.
127-127: Confirm alignment with required logging details.
You're using a new custom middleware (.UseRequestLogging()). Ensure it captures all relevant request details, including request headers, correlation IDs, and key telemetry attributes.
27-27: Ensure the middleware dependency is needed.
You added a reference to “Digdir.Domain.Dialogporten.WebApi.Common.Middleware.” Confirm that this namespace is specifically used for the custom request logging to avoid unnecessary using statements.
local-otel-configuration/loki-config.yaml (2)
1-23: Configuration looks comprehensive.
The Loki configuration is well-structured for local testing. Disabling auth (“auth_enabled: false”) might be fine for development, but confirm this remains disabled only in non-production environments.
24-45: Validate retention policies and resource usage.
Your environment sets a 168-hour (7-day) retention. If your log volume is high, consider adjusting storage configs or archiving strategies to avoid excessive disk usage.
Would you like me to script-check if any overrides are present in other config files?
local-otel-configuration/otel-collector-config.yaml (2)
28-29: Loki exporter addition recognized.
Good job adding “otlphttp” to forward logs to Loki. Ensure the endpoint remains consistent across the Docker and local environment.
54-54: Parallel logging exports.
Having both “otlphttp” and “debug” exporters ensures logs are forwarded to Loki while still allowing local debug. Confirm that this duplication aligns with your desired operational flow and doesn't degrade performance.
README.md (1)
Line range hint 129-226: Verify the accuracy of commands and endpoints.
The documentation is comprehensive and well-structured. Let's verify the accuracy of the provided information:
✅ Verification successful
Documentation accurately reflects the OpenTelemetry setup and endpoints
The verification confirms that:
- All documented ports in the README match exactly with the
docker-compose-otel.ymlconfiguration:- Grafana (3000)
- Loki (3100)
- Prometheus (9090)
- Jaeger (16686)
- OTLP receivers (4317/4318)
- Prometheus metrics (8888/8889)
- OpenTelemetry configuration variables are correctly referenced in the codebase and match the documented environment variables
- The docker-compose services (Grafana, Jaeger, Loki, Prometheus) are all present with correct configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and accessibility of documented endpoints and commands
# Check if docker-compose files exist and contain the documented services
echo "Verifying docker-compose configurations..."
rg -l "grafana|jaeger|loki|prometheus" docker-compose*.yml
# Check if the documented ports are consistently referenced
echo "Verifying port configurations..."
rg -A 2 "port.*3000|port.*3100|port.*9090|port.*16686|port.*4317|port.*4318|port.*8888|port.*8889" docker-compose*.yml
# Check if OpenTelemetry configuration is referenced in application code
echo "Verifying OpenTelemetry configuration usage..."
rg -l "OTEL_SERVICE_NAME|OTEL_EXPORTER_OTLP_ENDPOINT|OTEL_EXPORTER_OTLP_PROTOCOL"
Length of output: 936
Script:
#!/bin/bash
# Let's verify the specific port mappings in docker-compose-otel.yml
echo "Checking port mappings in docker-compose-otel.yml..."
cat docker-compose-otel.yml
# Let's also check the actual OpenTelemetry configuration in one of the services
echo -e "\nChecking OpenTelemetry configuration in WebApi Program.cs..."
cat "src/Digdir.Domain.Dialogporten.WebApi/Program.cs"
Length of output: 11227
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used
(MD034, no-bare-urls)
185-185: null
Bare URL used
(MD034, no-bare-urls)
192-192: null
Bare URL used
(MD034, no-bare-urls)
193-193: null
Bare URL used
(MD034, no-bare-urls)
208-208: null
Bare URL used
(MD034, no-bare-urls)
src/Digdir.Domain.Dialogporten.WebApi/Common/Middleware/RequestLoggingMiddleware.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
161-165: Consider adding health check endpoints for observability tools.While the local development setup is well documented, it would be beneficial to include health check endpoints for these services to help developers verify their setup.
Add the following section after the port listings:
+ Health check endpoints: + - Jaeger: http://localhost:16686/health + - Loki: http://localhost:3100/ready + - Prometheus: http://localhost:9090/-/healthy + - Grafana: http://localhost:3000/api/health
177-177: Fix bare URLs in the documentation.The Markdown linter has identified several bare URLs that should be properly formatted for better readability and maintainability.
Apply these changes:
- URL: http://localhost:16686 + URL: [http://localhost:16686](http://localhost:16686) - URL: http://localhost:9090 + URL: [http://localhost:9090](http://localhost:9090) - Direct URL: http://localhost:3100 - Grafana Integration: http://localhost:3000 + Direct URL: [http://localhost:3100](http://localhost:3100) + Grafana Integration: [http://localhost:3000](http://localhost:3000) - URL: http://localhost:3000 + URL: [http://localhost:3000](http://localhost:3000)Also applies to: 185-185, 192-193, 208-208
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used(MD034, no-bare-urls)
221-226: Consider adding configuration validation steps.The OpenTelemetry Collector endpoints section would benefit from including steps to validate the collector's configuration.
Add the following validation steps:
+ #### Validating Collector Configuration + To verify the collector is properly configured and receiving data: + + 1. Check collector health: + ```bash + curl http://localhost:8888/metrics | grep otelcol + ``` + + 2. Verify receivers are active: + ```bash + curl http://localhost:8888/metrics | grep receiver + ```
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)docker-compose-otel.yml(3 hunks)src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj
- docker-compose-otel.yml
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
177-177: null
Bare URL used
(MD034, no-bare-urls)
185-185: null
Bare URL used
(MD034, no-bare-urls)
192-192: null
Bare URL used
(MD034, no-bare-urls)
193-193: null
Bare URL used
(MD034, no-bare-urls)
208-208: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (2)
README.md (2)
Line range hint
129-138: LGTM! Comprehensive OpenTelemetry configuration documentation.The documentation clearly outlines the core features and configuration options for OpenTelemetry integration. The inclusion of environment variable examples is particularly helpful for developers.
199-205: LGTM! Helpful LogQL examples.The inclusion of practical LogQL query examples is valuable for developers getting started with log analysis.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (4)
Line range hint
27-41: Consider migrating bootstrap logger to OpenTelemetryWhile the main application logging is being migrated to OpenTelemetry, the bootstrap logger is still using Serilog. Consider migrating this to align with the new logging strategy.
42-49: Enhance error logging with structured propertiesConsider adding structured logging properties to the fatal error log to aid in debugging:
- Log.Fatal(ex, "Application terminated unexpectedly"); + Log.Fatal(ex, "Application terminated unexpectedly. {ExceptionType} {StackTrace}", + ex.GetType().Name, ex.StackTrace);
Line range hint
67-77: Document OpenTelemetry environment variablesThe following environment variables should be documented in the README or configuration documentation:
- OTEL_SERVICE_NAME
- OTEL_EXPORTER_OTLP_ENDPOINT
- OTEL_EXPORTER_OTLP_PROTOCOL
- OTEL_RESOURCE_ATTRIBUTES
Line range hint
29-42: Consider a phased approach to logging migrationThe current implementation shows a mix of logging approaches (Serilog for bootstrap logging, OpenTelemetry for application logging). Consider:
- Documenting the migration strategy
- Setting up a timeline for complete migration to OpenTelemetry
- Ensuring consistent logging patterns across the application
Also applies to: 67-77
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs(3 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
146-146: Verify RequestLogging middleware implementationPlease ensure that the new RequestLogging middleware captures all the necessary information that was previously logged by Serilog.RequestLogging.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
Line range hint
57-95: Consolidate OTLP endpoint logicThe logic around
OTEL_EXPORTER_OTLP_ENDPOINTis duplicated between the Serilog config block and theAddDialogportenTelemetryextension method. Consider centralizing the logic to avoid divergence or confusion in future maintenance.- var otlpEndpoint = context.Configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]; - if (!string.IsNullOrEmpty(otlpEndpoint)) { ... } + var otlpEndpoint = context.Configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]; + if (!string.IsNullOrEmpty(otlpEndpoint)) + { + // Possibly reference a shared method / utility that sets up Serilog + Telemetry + }
🧹 Nitpick comments (8)
local-otel-configuration/dashboards/runtime-metrics.json (4)
91-100: Good selection of memory metrics!The addition of Committed Memory and Working Set metrics provides valuable insights into the application's memory usage patterns. Consider adding "Gen 0/1/2 Sizes" metrics for more granular GC monitoring.
Add these metrics for better GC monitoring:
+ { + "expr": "dialogporten_process_runtime_dotnet_gc_size_bytes{generation=\"0\"}", + "legendFormat": "Gen 0 Size", + "refId": "D" + }, + { + "expr": "dialogporten_process_runtime_dotnet_gc_size_bytes{generation=\"1\"}", + "legendFormat": "Gen 1 Size", + "refId": "E" + }, + { + "expr": "dialogporten_process_runtime_dotnet_gc_size_bytes{generation=\"2\"}", + "legendFormat": "Gen 2 Size", + "refId": "F" + }
181-187: Consider adding generation-specific GC metricsWhile the current metrics provide good overall GC insights, adding generation-specific collection counts would help identify memory pressure patterns more effectively.
Add these metrics for generation-specific GC monitoring:
+ { + "expr": "rate(dialogporten_process_runtime_dotnet_gc_collections_count{generation=\"0\"}[5m])", + "legendFormat": "Gen 0 Collections/sec", + "refId": "C" + }, + { + "expr": "rate(dialogporten_process_runtime_dotnet_gc_collections_count{generation=\"1\"}[5m])", + "legendFormat": "Gen 1 Collections/sec", + "refId": "D" + }, + { + "expr": "rate(dialogporten_process_runtime_dotnet_gc_collections_count{generation=\"2\"}[5m])", + "legendFormat": "Gen 2 Collections/sec", + "refId": "E" + }
359-366: Consider adding exception type breakdownWhile the current metrics track overall exception rates, adding a breakdown by exception type would help identify specific problem areas more quickly.
Add exception type breakdown using labels:
{ - "expr": "rate(dialogporten_process_runtime_dotnet_exceptions_count_total[5m])", + "expr": "sum by (type) (rate(dialogporten_process_runtime_dotnet_exceptions_count_total{type!=\"\"}[5m]))", "legendFormat": "{{type}} Exceptions/sec", "refId": "A" }
Line range hint
1-368: Add dashboard descriptions and documentationThe dashboard would benefit from panel descriptions to help users understand what each metric represents and what values should trigger investigation.
Add description fields to each panel:
"title": "Memory Usage", + "description": "Shows various memory metrics including heap size, committed memory, and working set. High or growing values may indicate memory leaks.", "type": "timeseries", "title": "GC Collections", + "description": "Tracks garbage collection frequency and duration. Frequent collections or long GC pauses may impact performance.", "type": "timeseries", "title": "Thread Pool", + "description": "Monitors thread pool health. High queue length or thread count may indicate threading bottlenecks.", "type": "timeseries", "title": "Exceptions & Lock Contentions", + "description": "Tracks exception rates and lock contentions. Investigate sudden spikes or sustained high rates.", "type": "timeseries",src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (2)
38-72: Confirm sampling strategy for productionYou set an
AlwaysOnSampleronly ifenvironment.IsDevelopment(). Confirm whether you want a more selective sampler for production (e.g.,ParentBasedSampler) to strike a balance between data volume and diagnostic needs.
73-96: Check for consistent metrics across environmentsYou conditionally add Azure Monitor exporter if
APPLICATIONINSIGHTS_CONNECTION_STRINGis present; otherwise, you use OTLP. Confirm that metrics from both exporters give consistent views of performance and resource usage, especially if analyzing data in multiple tools.src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
49-49: Consider logging final config at startupCatching startup exceptions is great, but consider logging the active telemetry configuration as well (e.g., OTLP endpoint vs. console fallback) so operators can easily confirm the mode of operation.
110-110: Check for Redis or other external instrumentationYou call
AddDialogportenTelemetry(...), but if you also require Redis instrumentation or want to log Redis exceptions, confirm that your collector and instrumentation cover this external dependency.Would you like a snippet illustrating Redis instrumentation integration for more robust error logging?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
local-otel-configuration/dashboards/runtime-metrics.json(4 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(5 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1202
File: src/Digdir.Domain.Dialogporten.WebApi/Program.cs:159-162
Timestamp: 2024-11-12T05:32:45.312Z
Learning: Calling `AddOpenTelemetry()` multiple times is safe and does not cause duplicate service registrations.
🔇 Additional comments (10)
local-otel-configuration/dashboards/runtime-metrics.json (1)
275-279:⚠️ Potential issueRetain essential thread pool metrics
While tracking completed items is valuable, the thread count and queue length metrics are essential for monitoring thread pool health and detecting potential bottlenecks.
Keep the existing metrics alongside the new one:
{ "expr": "dialogporten_process_runtime_dotnet_thread_pool_queue_length", "legendFormat": "Queue Length", "refId": "A" }, { "expr": "dialogporten_process_runtime_dotnet_thread_pool_threads_count", "legendFormat": "Thread Count", "refId": "B" }, { "expr": "rate(dialogporten_process_runtime_dotnet_thread_pool_completed_items_count_total[5m])", "legendFormat": "Completed Items/sec", "refId": "C" }Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (2)
19-21: Ensure fallback behavior is intentionalThis early return is convenient if an OTLP endpoint is not defined, but consider logging or warning the user about missing telemetry configuration to avoid silent failure.
22-28: Validate explicit OTLP protocol valueThe switch statement covers
"grpc","http/protobuf", and"http", but be sure to handle or log unexpected protocol values clearly. It's good that you throw an exception, but also consider logging the supported options before throwing.src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (1)
5-5: Confirm removal of unused OpenTelemetry referencesYou now remove or omit the OpenTelemetry references from this project. Validate that no other classes in
Digdir.Library.Utils.AspNetrely on them to avoid runtime errors or missing instrumentation.src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
Line range hint
1-10: Simplified dependenciesBy removing the telemetry setup from this file, you've streamlined it to focus on health checks and logging. Ensure that consumers of
AspNetUtilitiesExtensionsare aware that telemetry configuration is no longer included here.src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (3)
11-11: Observe potential overlap in references
FastEndpoints.Swaggerremains in place, and you’ve introduced additional instrumentation-related packages. Verify they're all necessary and that no duplication or conflicting versions exist across the solution.
16-16: New Serilog + OpenTelemetry sinkThe addition of
Serilog.Sinks.OpenTelemetryis a solid approach for bridging your logs to OTLP. Make sure your OTLP collector is appropriately configured for logs in addition to metrics and traces for a unified view.
22-29: Align versions of OpenTelemetry packagesMultiple OpenTelemetry packages are added here. Keep their versions aligned to reduce the risk of missing features or subtle compatibility issues.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
16-36: Review environment variable usage for Azure MonitorYou import
Azure.Monitor.OpenTelemetry.Exporterand rely on environment variables such asAPPLICATIONINSIGHTS_CONNECTION_STRING. Confirm that your environment setup comprehensively covers all references (e.g., OTLP, AI) to prevent partial or misconfigured telemetry.
66-95: Guard log protocols thoroughlyWhen specifying
OtlpProtocol.GrpcorOtlpProtocol.HttpProtobuf, ensure the corresponding collector endpoints (gRPC vs. HTTP) match your actual collector. Consider logging an actionable error message if misconfiguration occurs.
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
39-44: Consider adding structured logging properties.The bootstrap logger could benefit from additional structured logging properties:
.MinimumLevel.Warning() .Enrich.WithEnvironmentName() .Enrich.FromLogContext() + .Enrich.WithMachineName() + .Enrich.WithProcessId() + .Enrich.WithThreadId() .WriteTo.OpenTelemetryOrConsole()
89-91: Add health checks for OpenTelemetry endpoint.Consider adding health checks for the OpenTelemetry endpoint to monitor the telemetry pipeline:
builder.Services .AddDialogportenTelemetry(builder.Configuration, builder.Environment) + .AddHealthChecks() + .AddUrlGroup(new Uri(builder.Configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]!), + name: "OpenTelemetry-Endpoint", + tags: new[] { "telemetry" }) + .Services
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1202
File: src/Digdir.Domain.Dialogporten.WebApi/Program.cs:159-162
Timestamp: 2024-11-12T05:32:45.312Z
Learning: Calling `AddOpenTelemetry()` multiple times is safe and does not cause duplicate service registrations.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (2)
1-22: LGTM! Well-structured extension method.The method signature is well-designed with appropriate dependencies and access level.
35-100: 🛠️ Refactor suggestionAdd Redis instrumentation for comprehensive monitoring.
The PR objectives mention Redis exceptions, but Redis instrumentation is not configured. Consider adding StackExchange.Redis instrumentation:
.AddEntityFrameworkCoreInstrumentation() .AddNpgsql() .AddFusionCacheInstrumentation() + .AddRedisInstrumentation() .AddOtlpExporter(options =>Also, ensure to dispose of the TracerProvider to prevent memory leaks:
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Outdated
Show resolved
Hide resolved
… environments than Development on final logger.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (1)
35-35:⚠️ Potential issueAdd URI validation for the OTLP endpoint
The URI creation could throw an exception if the endpoint string is malformed.
Apply this diff:
- var endpoint = new Uri(configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]!); + if (!Uri.TryCreate(configuration["OTEL_EXPORTER_OTLP_ENDPOINT"], UriKind.Absolute, out var endpoint)) + { + throw new ArgumentException($"Invalid OTLP endpoint URI: {configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]}"); + }
🧹 Nitpick comments (4)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (4)
17-18: Consider moving configuration constants to a dedicated classThese OpenTelemetry configuration constants could be moved to a dedicated configuration class to maintain consistency with other configuration keys and improve maintainability.
28-33: Consider using pattern matching for protocol parsingThe current switch expression could be more robust with explicit pattern matching.
- var otlpProtocol = configuration["OTEL_EXPORTER_OTLP_PROTOCOL"]?.ToLowerInvariant() switch - { - "grpc" => OtlpExportProtocol.Grpc, - "http/protobuf" => OtlpExportProtocol.HttpProtobuf, - _ => throw new ArgumentException($"Unsupported protocol: {configuration["OTEL_EXPORTER_OTLP_PROTOCOL"]}") - }; + var otlpProtocol = configuration["OTEL_EXPORTER_OTLP_PROTOCOL"] switch + { + string protocol when protocol.Equals("grpc", StringComparison.OrdinalIgnoreCase) + => OtlpExportProtocol.Grpc, + string protocol when protocol.Equals("http/protobuf", StringComparison.OrdinalIgnoreCase) + => OtlpExportProtocol.HttpProtobuf, + null => throw new ArgumentNullException("OTEL_EXPORTER_OTLP_PROTOCOL", "Protocol must be specified"), + var protocol => throw new ArgumentException($"Unsupported protocol: {protocol}", "OTEL_EXPORTER_OTLP_PROTOCOL") + };
111-118: Consider adding protocol validation messageWhen protocol parsing fails, it would be helpful to include the valid protocol options in the error message.
not null when Enum.TryParse<OtlpProtocol>(otelProtocol, out var protocol) && Uri.IsWellFormedUriString(otelEndpoint, UriKind.Absolute) => writeTo.OpenTelemetry(options => { options.Endpoint = otelEndpoint; options.Protocol = protocol; }), - _ => throw new InvalidOperationException($"Invalid otel config. Endpoint: {otelEndpoint}, Protocol: {otelProtocol}") + _ => throw new InvalidOperationException( + $"Invalid OpenTelemetry configuration. Endpoint: {otelEndpoint}, Protocol: {otelProtocol}. " + + $"Valid protocols are: {string.Join(", ", Enum.GetNames<OtlpProtocol>())}")
123-139: Consider consolidating duplicate OpenTelemetry configuration logicThis method shares similar logic with
OpenTelemetryOrConsole. Consider extracting the common configuration logic into a private method.+ private static (string? endpoint, string? protocol) GetOpenTelemetryConfig( + Func<string, string?> configProvider) + { + return ( + configProvider(OtelExporterOtlpEndpoint), + configProvider(OtelExporterOtlpProtocol) + ); + } + + private static LoggerConfiguration ConfigureOpenTelemetry( + LoggerConfiguration config, + string? endpoint, + string? protocol, + Action<LoggerConfiguration> onInvalid) + { + return endpoint switch + { + not null when + Enum.TryParse<OtlpProtocol>(protocol, out var parsedProtocol) + && Uri.IsWellFormedUriString(endpoint, UriKind.Absolute) + => config.WriteTo.OpenTelemetry(options => + { + options.Endpoint = endpoint; + options.Protocol = parsedProtocol; + }), + _ => onInvalid(config) + }; + }Then update the methods to use these helpers:
public static LoggerConfiguration TryWriteToOpenTelemetry( this LoggerConfiguration config) { - var otelEndpoint = Environment.GetEnvironmentVariable(OtelExporterOtlpEndpoint); - var otelProtocol = Environment.GetEnvironmentVariable(OtelExporterOtlpProtocol); - return otelEndpoint switch - { - not null when - Enum.TryParse<OtlpProtocol>(otelProtocol, out var protocol) - && Uri.IsWellFormedUriString(otelEndpoint, UriKind.Absolute) - => config.WriteTo.OpenTelemetry(options => - { - options.Endpoint = otelEndpoint; - options.Protocol = protocol; - }), - _ => config - }; + var (endpoint, protocol) = GetOpenTelemetryConfig( + Environment.GetEnvironmentVariable); + return ConfigureOpenTelemetry( + config, + endpoint, + protocol, + c => c); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1202
File: src/Digdir.Domain.Dialogporten.WebApi/Program.cs:159-162
Timestamp: 2024-11-12T05:32:45.312Z
Learning: Calling `AddOpenTelemetry()` multiple times is safe and does not cause duplicate service registrations.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (2)
51-68: LGTM: Well-implemented instrumentation filteringThe HTTP client instrumentation filtering logic correctly handles Azure.Core.Http activities to prevent duplicate telemetry.
84-101: LGTM: Proper fallback configuration for metrics exportThe code appropriately handles both Azure Monitor and OTLP metric export configurations with proper fallback logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (3)
40-41: Add default service name for better observability.Consider providing a meaningful default service name when both configuration and environment name are null:
- serviceName: configuration["OTEL_SERVICE_NAME"] ?? environment.ApplicationName); + serviceName: configuration["OTEL_SERVICE_NAME"] + ?? environment.ApplicationName + ?? "dialogporten-webapi");
59-67: Simplify HTTP client instrumentation filter.The current filter logic can be simplified for better readability:
- o.FilterHttpRequestMessage = _ => - { - var parentActivity = Activity.Current?.Parent; - if (parentActivity != null && parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal)) - { - return false; - } - return true; - }; + o.FilterHttpRequestMessage = _ => + Activity.Current?.Parent?.Source.Name != "Azure.Core.Http";
74-76: Extract common OTLP exporter configuration.The OTLP exporter configuration is duplicated between traces and metrics. Consider extracting it:
+ private static void ConfigureOtlpExporter(OtlpExporterOptions options, Uri baseEndpoint, string path, OtlpExportProtocol protocol) + { + options.Endpoint = new Uri(baseEndpoint, path); + options.Protocol = protocol; + } // In tracing configuration - options.Endpoint = new Uri(endpoint, "/v1/traces"); - options.Protocol = otlpProtocol; + ConfigureOtlpExporter(options, endpoint, "/v1/traces", otlpProtocol); // In metrics configuration - options.Endpoint = new Uri(endpoint, "/v1/metrics"); - options.Protocol = otlpProtocol; + ConfigureOtlpExporter(options, endpoint, "/v1/metrics", otlpProtocol);Also applies to: 96-98
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/OpenTelemetryExtensions.cs
- src/Digdir.Domain.Dialogporten.Service/Common/Extensions/OpenTelemetryExtensions.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build-and-test
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (3)
1-19: LGTM! Well-structured class setup with appropriate constants.The class is properly organized with all necessary imports and well-named constants for OpenTelemetry configuration.
35-35: 🛠️ Refactor suggestionAdd null check and validation for OTLP endpoint.
The URI creation could throw a NullReferenceException. Consider adding proper validation:
- var endpoint = new Uri(configuration[OtelExporterOtlpEndpoint]!); + var endpointStr = configuration[OtelExporterOtlpEndpoint]; + if (string.IsNullOrEmpty(endpointStr)) + { + throw new ArgumentException("OTLP endpoint cannot be null or empty"); + } + var endpoint = new Uri(endpointStr);Likely invalid or redundant comment.
51-55: Verify if request logging configuration is sufficient.Based on the PR objectives, there was uncertainty about replacing request logging with middleware. The current ASP.NET Core instrumentation configuration looks good but verify if it captures all necessary request information:
✅ Verification successful
Request logging configuration is properly implemented
The current OpenTelemetry instrumentation combined with Serilog configuration provides comprehensive request logging capabilities:
- HTTP request/response tracking via OpenTelemetry instrumentation
- Exception capture enabled
- Structured logging with proper context
- Integration with the observability stack
- Health endpoints properly filtered
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing request logging middleware or Serilog request logging rg -l "UseRequestLogging|UseSerilogRequestLogging" # Search for custom request logging middleware ast-grep --pattern 'class $_ : IMiddleware { $$$ public async Task InvokeAsync($_, $_) { $$$ } }'Length of output: 222
Script:
#!/bin/bash # Check the CDC Program.cs request logging implementation cat src/Digdir.Domain.Dialogporten.ChangeDataCapture/Program.cs # Search for any logging configuration in WebApi rg -l "AddLogging|ConfigureLogging|ILogger|LoggerConfiguration" src/Digdir.Domain.Dialogporten.WebApi/Length of output: 4677
Script:
#!/bin/bash # Check WebApi Program.cs for logging configuration cat src/Digdir.Domain.Dialogporten.WebApi/Program.cs # Also check if there are any other middleware configurations rg -A 5 "app\.Use|builder\.Use" src/Digdir.Domain.Dialogporten.WebApi/Program.csLength of output: 9592
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (5)
17-18: Consider moving configuration constants to a dedicated class.These OpenTelemetry configuration constants could be moved to a dedicated configuration class (e.g.,
TelemetryConstants) to centralize all configuration keys.
56-68: Extract HTTP client filter logic to a separate method.The HTTP client filtering logic is complex enough to warrant its own method for better readability and maintainability.
+ private static bool ShouldInstrumentHttpRequest(HttpRequestMessage _) + { + var parentActivity = Activity.Current?.Parent; + return parentActivity == null || + !parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal); + } .AddHttpClientInstrumentation(o => { o.RecordException = true; - o.FilterHttpRequestMessage = _ => - { - var parentActivity = Activity.Current?.Parent; - if (parentActivity != null && parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal)) - { - return false; - } - return true; - }; + o.FilterHttpRequestMessage = ShouldInstrumentHttpRequest; })
94-99: Consider extracting OTLP exporter configuration.The OTLP exporter configuration is duplicated between traces and metrics. Consider extracting to a helper method.
+ private static void ConfigureOtlpMetricsExporter(OtlpExporterOptions options, Uri endpoint, OtlpExportProtocol protocol) + { + options.Endpoint = new Uri(endpoint, "/v1/metrics"); + options.Protocol = protocol; + } - metrics.AddOtlpExporter(options => - { - options.Endpoint = new Uri(endpoint, "/v1/metrics"); - options.Protocol = otlpProtocol; - }); + metrics.AddOtlpExporter(options => ConfigureOtlpMetricsExporter(options, endpoint, otlpProtocol));
115-115: Enhance error message for invalid OTLP endpoint.The current error message could be more descriptive to help with troubleshooting.
- _ => throw new InvalidOperationException($"Invalid otel endpoint: {otelEndpoint}") + _ => throw new InvalidOperationException( + $"Invalid OTLP endpoint URL format: {otelEndpoint}. " + + "The endpoint must be a valid absolute URI (e.g., http://localhost:4317).")
124-127: Consider logging when skipping OpenTelemetry configuration.Silent failure might make it harder to debug configuration issues in production.
if (otelEndpoint is null || !Uri.IsWellFormedUriString(otelEndpoint, UriKind.Absolute)) { + // Log warning only if endpoint is provided but invalid + if (otelEndpoint is not null) + { + Log.Warning("Skipping OpenTelemetry configuration due to invalid endpoint: {Endpoint}", otelEndpoint); + } return config; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj(0 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Digdir.Domain.Dialogporten.Service/Common/Extensions/OpenTelemetryExtensions.cs
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/OpenTelemetryExtensions.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build-and-test
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs (2)
140-156: LGTM! Well-structured helper methods.The helper methods are well-implemented with clear responsibilities, appropriate error handling, and good encapsulation.
84-91: Verify secure handling of Application Insights connection string.Ensure the connection string is properly secured in configuration and not logged or exposed in telemetry.
✅ Verification successful
Connection string handling follows security best practices ✅
The Application Insights connection string is properly secured:
- Retrieved from environment variables
- No logging or exposure in telemetry
- Used only for configuring the Azure Monitor exporter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential connection string logging rg -i "applicationinsights.*connection.*string" -g "log|console.write|debug|trace"Length of output: 233
Script:
#!/bin/bash # Search for any mentions of ApplicationInsights connection string in all files rg -i "applicationinsights.*connection.*string" -A 2 -B 2 # Check configuration files for connection string patterns fd -e json -e xml -e config --exec grep -i "applicationinsights" {} # Look for general connection string handling patterns ast-grep --pattern 'options.ConnectionString = $_'Length of output: 2457
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/OpenTelemetryExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Service/Common/Extensions/OpenTelemetryExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/OpenTelemetryExtensions.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
README.md (1)
Line range hint
52-54: Fix inconsistent localhost URLs in the documentation.The localhost URLs in the documentation are inconsistent. All three URLs point to the same swagger endpoint (7214) instead of their respective ports (7124, 7215, 7216).
Apply this diff to fix the URLs:
-* WebAPI/SwaggerUI: [localhost:7124/swagger](https://localhost:7214/swagger/index.html) -* GraphQl/BananaCakePop: [localhost:7215/graphql](https://localhost:7214/swagger/index.html) -* Redis/Insight: [localhost:7216](https://localhost:7214/swagger/index.html) +* WebAPI/SwaggerUI: [localhost:7124/swagger](https://localhost:7124/swagger/index.html) +* GraphQl/BananaCakePop: [localhost:7215/graphql](https://localhost:7215/graphql) +* Redis/Insight: [localhost:7216](https://localhost:7216)
🧹 Nitpick comments (3)
README.md (2)
177-178: Format bare URLs as proper markdown links.Convert bare URLs to proper markdown links for better documentation consistency.
Apply this diff to fix the URL formatting:
-URL: http://localhost:16686 +URL: [http://localhost:16686](http://localhost:16686) -URL: http://localhost:9090 +URL: [http://localhost:9090](http://localhost:9090) -Direct URL: http://localhost:3100 -Grafana Integration: http://localhost:3000 (preferred interface) +Direct URL: [http://localhost:3100](http://localhost:3100) +Grafana Integration: [http://localhost:3000](http://localhost:3000) (preferred interface)Also applies to: 185-186, 192-194
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used(MD034, no-bare-urls)
199-205: Enhance LogQL examples with comments.The LogQL examples would benefit from more detailed explanations of what each query does and when to use them.
Apply this diff to enhance the examples:
```logql - # Example: Find all error logs + # Example 1: Find all error logs from web-api container + # Use this to quickly identify errors in the API service {container="web-api"} |= "error" - # Example: Find logs with specific trace ID + # Example 2: Find logs with specific trace ID across web-api and graphql services + # Use this to correlate logs across services for a specific request {container=~"web-api|graphql"} |~ "trace_id=([a-f0-9]{32})" ```src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (1)
43-43: Consider adding startup telemetryWhile removing TelemetryConfiguration simplifies the code, consider adding explicit startup telemetry to track successful service initialization.
static void BuildAndRun(string[] args) { + Activity.Current?.AddEvent(new("ServiceStarting")); var builder = WebApplication.CreateBuilder(args); // ... existing code ... app.Run(); + Activity.Current?.AddEvent(new("ServiceStarted")); }Also applies to: 31-31
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(2 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(4 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(0 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
177-177: null
Bare URL used
(MD034, no-bare-urls)
185-185: null
Bare URL used
(MD034, no-bare-urls)
192-192: null
Bare URL used
(MD034, no-bare-urls)
193-193: null
Bare URL used
(MD034, no-bare-urls)
208-208: null
Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build-and-test
🔇 Additional comments (8)
README.md (2)
Line range hint
308-341: Excellent documentation of the local development settings!The documentation for
appsettings.local.jsonis particularly well-structured and helpful:
- Clear explanation of when to use local vs. shared settings
- Good example of configuration override
- Clear instructions for adding to new projects
Line range hint
1-600: Outstanding documentation update!The README now provides comprehensive coverage of:
- Local development setup for both Mac and Windows
- Detailed observability stack configuration
- Clear deployment processes and guidelines
- Well-structured configuration management
This will significantly improve the developer experience and reduce onboarding friction.
🧰 Tools
🪛 Markdownlint (0.37.0)
177-177: null
Bare URL used(MD034, no-bare-urls)
185-185: null
Bare URL used(MD034, no-bare-urls)
192-192: null
Bare URL used(MD034, no-bare-urls)
193-193: null
Bare URL used(MD034, no-bare-urls)
208-208: null
Bare URL used(MD034, no-bare-urls)
src/Digdir.Domain.Dialogporten.Service/Program.cs (3)
18-20: LGTM: Logging enrichment and configurationThe addition of
FromLogContext()enrichment and proper configuration of logging providers looks good. This ensures consistent log context across the application.Also applies to: 50-51
41-43: LGTM: Configuration setupThe configuration setup properly handles both Azure and local environments.
37-37: Verify logging behavior after removing Application InsightsThe removal of the TelemetryConfiguration parameter simplifies the startup, but we need to ensure all critical logs are properly captured.
#!/bin/bash # Check for any remaining Application Insights references in the Service project rg "ApplicationInsights" --type cs src/Digdir.Domain.Dialogporten.Service/ # Check OpenTelemetry configuration rg "OpenTelemetry" --type cs -C 3 src/Digdir.Domain.Dialogporten.Service/Also applies to: 25-25
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (3)
24-26: LGTM: Bootstrap logger configurationThe bootstrap logger is properly configured with OpenTelemetry integration.
47-49: LGTM: Configuration setupThe configuration setup properly handles both Azure and local environments, maintaining consistency with other services.
56-57: Verify GraphQL-specific logging requirementsWhile the OpenTelemetry integration looks good, ensure that GraphQL-specific logging requirements (query execution, resolver timing, etc.) are properly captured.
✅ Verification successful
GraphQL logging requirements are properly implemented ✅
The codebase has comprehensive GraphQL logging through:
- OpenTelemetry integration via
ExecutionDiagnosticEventListener- Distributed tracing with dedicated
ActivitySource- Standard logging patterns with
ILogger🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for GraphQL-specific logging configuration rg "IRequestExecutor|IOperationContext" --type cs src/Digdir.Domain.Dialogporten.GraphQL/ echo "=== Checking OpenTelemetry integration with GraphQL ===" rg "AddInstrumentation.*HotChocolate|UseActivity" --type cs src/Digdir.Domain.Dialogporten.GraphQL/Length of output: 314
Script:
#!/bin/bash # Search for GraphQL diagnostic events and middleware logging echo "=== Checking for GraphQL diagnostic events ===" rg "DiagnosticEventListener|IEventListener" --type cs src/Digdir.Domain.Dialogporten.GraphQL/ echo "=== Checking for GraphQL middleware and logging ===" rg "UseField|Use\(.*Log|AddMiddleware.*Log" --type cs src/Digdir.Domain.Dialogporten.GraphQL/ echo "=== Checking for custom logging extensions ===" rg "ILogger<|LogInformation|LogDebug|LogError" -A 2 --type cs src/Digdir.Domain.Dialogporten.GraphQL/Length of output: 1521
Script:
#!/bin/bash # Check OpenTelemetryEventListener implementation echo "=== Checking OpenTelemetryEventListener implementation ===" rg -A 10 "class OpenTelemetryEventListener" --type cs src/Digdir.Domain.Dialogporten.GraphQL/Length of output: 1363
Description
Notes:
ServiceRelated Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)