feat(apps): add otel exporter for graphql, service and web-api #1528
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive configuration for monitoring and observability using OpenTelemetry, Jaeger, Prometheus, and Grafana. It includes a new Changes
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
|
arealmaas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (10)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (3)
86-92: Validate and Parse Protocol Using Enum ParsingSimplify protocol parsing by using
Enum.TryParseforOtlpExportProtocol. This approach enhances readability and maintainability.Consider applying this change:
- var otlpProtocol = settings.Protocol.ToLowerInvariant() switch - { - "grpc" => OtlpExportProtocol.Grpc, - "http/protobuf" => OtlpExportProtocol.HttpProtobuf, - "http" => OtlpExportProtocol.HttpProtobuf, - _ => throw new ArgumentException($"Unsupported protocol: {settings.Protocol}") - }; + if (!Enum.TryParse<OtlpExportProtocol>(settings.Protocol, true, out var otlpProtocol)) + { + throw new ArgumentException($"Unsupported protocol: {settings.Protocol}"); + }
131-132: Use Logging Framework for ConsistencyReplace
Console.WriteLinewithILoggerto maintain consistent logging practices throughout the application.Apply this diff:
- Console.WriteLine("[OpenTelemetry] OTLP exporter not configured - skipping"); + logger.LogWarning("OTLP exporter not configured - skipping");
154-163: MakeTraceSourcesConfigurableHardcoding trace source names can limit flexibility. Consider making
TraceSourcesconfigurable via app settings to allow easier adjustments without code changes.Example adjustment:
- public HashSet<string> TraceSources { get; set; } = new() - { - AzureSource, - MassTransitSource - }; + public HashSet<string> TraceSources { get; set; } + + public TelemetrySettings() + { + TraceSources = new HashSet<string> + { + AzureSource, + MassTransitSource + }; + }Then, allow injection of
TraceSourcesvia configuration.local-otel-configuration/grafana-datasources.yml (1)
8-8: Remove Trailing Spaces and Add Newline at End of FileThe file contains trailing spaces and is missing a newline at the end, which can cause issues with some parsers or tools.
Apply this diff to fix the formatting:
- isDefault: true + isDefault: true +🧰 Tools
🪛 yamllint (1.35.1)
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
local-otel-configuration/otel-collector-config.yaml (2)
25-27: Reassess Debug Exporter's Sampling ConfigurationThe
debugexporter withverbosity: detailedcan generate a large volume of logs, impacting performance.Consider adjusting the sampling rates or using it conditionally in development environments.
36-39: Set Appropriate Logging Levels for ProductionCurrently, the telemetry logs are set to
debuglevel withdevelopment: true. In a production environment, this should be adjusted to reduce verbosity.Adjust the logging configuration based on the environment:
telemetry: logs: level: info development: falselocal-otel-configuration/dashboards/aspnet-core-metrics.json (1)
1-176: Enhance dashboard with additional critical metrics.While the current metrics provide good basic monitoring, consider adding these essential metrics for comprehensive monitoring:
Add these panels to the dashboard:
{ "panels": [ // ... existing panels ... + { + "title": "HTTP Response Status Codes", + "type": "timeseries", + "targets": [{ + "expr": "sum(rate(dialogporten_http_server_requests_total[$__rate_interval])) by (status_code)", + "legendFormat": "{{status_code}}" + }] + }, + { + "title": "Error Rate", + "type": "timeseries", + "targets": [{ + "expr": "sum(rate(dialogporten_http_server_requests_total{status_code=~\"5.*\"}[$__rate_interval])) / sum(rate(dialogporten_http_server_requests_total[$__rate_interval])) * 100", + "legendFormat": "Error Rate %" + }] + } ] }local-otel-configuration/dashboards/http-client-metrics.json (1)
1-296: Add failure monitoring metrics to the dashboard.The dashboard provides good performance monitoring but lacks visibility into failures and timeouts.
Add these panels to track failures:
{ "panels": [ // ... existing panels ... + { + "title": "Failed Requests by Reason", + "type": "timeseries", + "targets": [{ + "expr": "sum(rate(dialogporten_http_client_requests_failed_total[$__rate_interval])) by (reason)", + "legendFormat": "{{reason}}" + }] + }, + { + "title": "Request Timeouts", + "type": "timeseries", + "targets": [{ + "expr": "sum(rate(dialogporten_http_client_requests_failed_total{reason=\"timeout\"}[$__rate_interval]))", + "legendFormat": "Timeouts" + }] + } ] }local-otel-configuration/dashboards/runtime-metrics.json (1)
84-96: Enhance memory metrics panel with additional indicators.Consider adding these essential memory metrics to provide a more comprehensive view:
Add these targets to the Memory Usage panel:
"targets": [ { "datasource": { "type": "prometheus", "uid": "Prometheus" }, "expr": "dialogporten_process_runtime_dotnet_gc_heap_size_bytes", "legendFormat": "Heap Size", "refId": "A" - } + }, + { + "datasource": { + "type": "prometheus", + "uid": "Prometheus" + }, + "expr": "process_working_set_bytes", + "legendFormat": "Working Set", + "refId": "B" + } ]local-otel-configuration/grafana-dashboards.yml (1)
1-11: Configuration looks good, minor YAML fixes needed.The Grafana dashboard provisioning configuration is correctly structured. However, there are two minor YAML formatting issues to fix:
Apply this diff to fix the YAML formatting:
apiVersion: 1 providers: - name: 'Default' orgId: 1 folder: '' type: file disableDeletion: false editable: true options: - path: /etc/grafana/provisioning/dashboards + path: /etc/grafana/provisioning/dashboards +🧰 Tools
🪛 yamllint (1.35.1)
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (16)
docker-compose-otel.yml(1 hunks)docker-compose.yml(4 hunks)local-otel-configuration/dashboards/aspnet-core-metrics.json(1 hunks)local-otel-configuration/dashboards/http-client-metrics.json(1 hunks)local-otel-configuration/dashboards/runtime-metrics.json(1 hunks)local-otel-configuration/grafana-dashboards.yml(1 hunks)local-otel-configuration/grafana-datasources.yml(1 hunks)local-otel-configuration/otel-collector-config.yaml(1 hunks)local-otel-configuration/prometheus.yml(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/appsettings.json(1 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 (2)
- src/Digdir.Domain.Dialogporten.WebApi/appsettings.json
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
🧰 Additional context used
📓 Learnings (2)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1349
File: src/Digdir.Domain.Dialogporten.GraphQL/Program.cs:70-74
Timestamp: 2024-11-12T05:32:45.312Z
Learning: In this project, the OpenTelemetry configurations are handled in the `.ConfigureTelemetry()` method.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1349
File: src/Digdir.Domain.Dialogporten.GraphQL/Program.cs:70-74
Timestamp: 2024-11-12T05:32:45.312Z
Learning: In this project, the OpenTelemetry configurations are handled in the `.ConfigureTelemetry()` method.
🪛 yamllint (1.35.1)
local-otel-configuration/grafana-datasources.yml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
local-otel-configuration/grafana-dashboards.yml
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
local-otel-configuration/prometheus.yml
[error] 8-8: no new line character at the end of file
(new-line-at-end-of-file)
[error] 8-8: trailing spaces
(trailing-spaces)
🔇 Additional comments (4)
local-otel-configuration/otel-collector-config.yaml (1)
17-19: Confirm Prometheus Exporter Endpoint and Namespace
Ensure that the Prometheus exporter endpoint and namespace align with your infrastructure and naming conventions.
Run the following script to verify that the endpoint is correctly set and not conflicting with other services:
✅ Verification successful
Let me gather more information about the Prometheus configuration in the codebase.
Prometheus configuration is correctly set and aligned with the infrastructure
The Prometheus exporter configuration is properly configured and integrated:
- Port 8889 is consistently used across the OpenTelemetry setup:
- Exposed in docker-compose-otel.yml
- Configured as target in prometheus.yml
- Set as endpoint in otel-collector-config.yaml
- The "dialogporten" namespace is unique to this collector configuration
- The configuration aligns with the overall monitoring setup including Grafana and Prometheus services
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if port 8889 is already in use and validate the namespace.
# Test: Check for services using port 8889
lsof -i :8889
# Expected Result: No other services should be using port 8889.
# Verify the namespace in existing Prometheus configurations
rg 'namespace: "dialogporten"' local-otel-configuration/
Length of output: 251
Script:
#!/bin/bash
# Search for any other Prometheus configurations or port definitions
rg -g '*.yaml' -g '*.yml' 'prometheus:' -A 5
# Search for any port 8889 usage across the codebase
rg '8889'
# Look for any other OpenTelemetry collector configurations
fd -e yaml -e yml | grep -i 'collector'
Length of output: 2560
src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (1)
14-21: Ensure Compatibility of New Package References
The addition of OpenTelemetry.Exporter.OpenTelemetryProtocol and ZiggyCreatures.FusionCache.OpenTelemetry packages should be compatible with existing dependencies and the .NET target framework.
Run the following script to check for compatibility issues:
Do you want assistance in resolving any compatibility issues that may arise?
✅ Verification successful
OpenTelemetry Package Dependencies are Compatible
The OpenTelemetry packages are properly aligned and compatible:
- Core
OpenTelemetrypackage (1.10.0) matches the version of its exporterOpenTelemetry.Exporter.OpenTelemetryProtocol(1.10.0) Azure.Monitor.OpenTelemetry.AspNetCore(1.2.0) is a stable version compatible with .NET packagesZiggyCreatures.FusionCache.OpenTelemetry(1.4.1) is compatible with the core OpenTelemetry version- The project already has proper OpenTelemetry integration as evidenced by the usage in
AspNetUtilitiesExtensions.cs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package compatibility.
# List all package references with versions
cat src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj | grep '<PackageReference'
# Check for any known conflicts or required updates
dotnet list package --outdated
Length of output: 1345
Script:
#!/bin/bash
# Let's check the target framework and other relevant information from the project file
cat src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj
# Also check if there are any OpenTelemetry related configurations or usages
rg "OpenTelemetry" -A 3 -B 3
Length of output: 10641
docker-compose.yml (1)
18-18: Verify scale reduction impact
Reducing scale from 2 to 1 for webapi could impact service availability and load handling capability.
✅ Verification successful
Let me search for more specific information about scaling configuration.
Let me check the deployment configuration in infrastructure code to understand if there are other scaling mechanisms in place.
Based on the findings from the CHANGELOG.md and docker-compose.yml, I can now provide a final response:
Scale reduction appears to be part of a broader infrastructure change
The scale reduction from 2 to 1 in docker-compose.yml is likely intentional as the application has been migrated to container apps with proper CPU and memory scalers:
- Recent changes show implementation of container app deployment with dedicated scaling configuration (feat(service): deploy application in container apps #1303)
- CPU and memory scalers were added (feat(applications): add scalers for cpu and memory #1295)
- Scale configuration was subsequently corrected (fix(applications): use correct scale configuration #1311)
The docker-compose.yml is likely used for local development, where high availability is not a primary concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for load balancing or high availability requirements
rg -A 5 "scale.*[2-9]|replicas.*[2-9]|high.availability|load.balancing"
Length of output: 7541
Script:
#!/bin/bash
# Check for scale configuration in infrastructure code and deployment files
rg -A 5 "scale:|replicas:|minReplicas|maxReplicas" --type yaml
Length of output: 560
Script:
#!/bin/bash
# Check infrastructure code for container app scaling configuration
rg -A 10 "containerApp|scale" --type bicep --type terraform
Length of output: 91
local-otel-configuration/grafana-dashboards.yml (1)
11-11: Verify dashboard path configuration in Docker setup.
The dashboard path /etc/grafana/provisioning/dashboards needs to be properly mounted in the Grafana container.
✅ Verification successful
Dashboard path configuration is correctly set up in Docker environment
The verification confirms that:
- The dashboard path is properly mounted in
docker-compose-otel.ymlwith both the configuration and dashboard files - Dashboard JSON files exist in the expected location at
local-otel-configuration/dashboards/
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the dashboard path is correctly configured in Docker Compose
# Check if the dashboard path is mounted in docker-compose-otel.yml
rg -A 5 "/etc/grafana/provisioning/dashboards"
# Check if dashboard JSON files exist
fd -e json . local-otel-configuration/dashboards/
Length of output: 619
🧰 Tools
🪛 yamllint (1.35.1)
[error] 11-11: no new line character at the end of file
(new-line-at-end-of-file)
[error] 11-11: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
56-58: Consider validating OTEL protocol format early.The protocol value is used later in the configuration but isn't validated at the point of assignment. Early validation would provide clearer error messages.
- settings.Protocol = configuration["OTEL_EXPORTER_OTLP_PROTOCOL"]; + var protocol = configuration["OTEL_EXPORTER_OTLP_PROTOCOL"]; + if (!string.IsNullOrEmpty(protocol) && + !new[] { "grpc", "http/protobuf", "http" }.Contains(protocol.ToLowerInvariant())) + { + throw new ArgumentException($"Unsupported protocol: {protocol}. " + + "Supported values are: grpc, http/protobuf, http"); + } + settings.Protocol = protocol;src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
115-127: Simplify HTTP client instrumentation filter.The current implementation can be made more concise while maintaining readability.
.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 = _ => + Activity.Current?.Parent?.Source.Name != "Azure.Core.Http"; })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1528
File: src/Digdir.Domain.Dialogporten.Service/Program.cs:56-58
Timestamp: 2024-12-10T10:54:37.635Z
Learning: In `src/Digdir.Domain.Dialogporten.Service/Program.cs`, when running locally without Docker, OTEL settings (`OTEL_SERVICE_NAME`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_PROTOCOL`) are optional and may not be provided, so validation for these settings is not required.
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
61-84: LGTM! Robust attribute parsing implementation.
The implementation uses HttpUtility.ParseQueryString for parsing resource attributes, which is more resilient than simple string splitting. Good error handling with clear error messages.
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
152-167: LGTM! Well-structured telemetry settings class.
The TelemetrySettings class provides a clean interface for configuring telemetry with sensible defaults for trace sources.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
81-112: LGTM! Consistent telemetry configuration.
The implementation maintains consistency with other services while properly handling null values and attribute parsing.
81-86: Verify OpenTelemetry configuration in docker-compose files.
Since this is for local OpenTelemetry setup, ensure that the environment variables are properly configured in docker-compose files.
✅ Verification successful
OpenTelemetry configuration is properly set up in docker-compose.yml
The verification confirms that the OpenTelemetry environment variables are correctly configured in docker-compose.yml for both the WebAPI and GraphQL services:
OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-collector:4318OTEL_EXPORTER_OTLP_PROTOCOL=httpOTEL_SERVICE_NAMEis set appropriately for each service- Additional attributes and debug logging are also configured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if OTEL environment variables are configured in docker-compose files
# Look for docker-compose files
echo "Checking docker-compose files for OTEL configuration:"
fd --type f "docker-compose.*\.ya?ml$" | while read -r file; do
echo "\nChecking $file:"
# Check for OTEL environment variables
rg -A 5 "OTEL_(SERVICE_NAME|EXPORTER_OTLP_ENDPOINT|EXPORTER_OTLP_PROTOCOL)" "$file"
done
Length of output: 1830
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (2)
115-127: Improve HTTP client instrumentation configuration structureThe current nested configuration structure could be improved for better readability and maintainability.
Consider this restructuring:
- .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; - }; - }) + .AddHttpClientInstrumentation(o => + { + o.RecordException = true; + o.FilterHttpRequestMessage = ShouldTraceRequest; + })Add this helper method:
private static bool ShouldTraceRequest(HttpRequestMessage _) { var parentActivity = Activity.Current?.Parent; return parentActivity?.Source.Name != "Azure.Core.Http"; }
152-167: Add XML documentation to TelemetrySettings classThe
TelemetrySettingsclass would benefit from XML documentation to explain its purpose and properties.Consider adding documentation:
+/// <summary> +/// Configuration settings for OpenTelemetry integration. +/// </summary> public class TelemetrySettings { private const string MassTransitSource = "MassTransit"; private const string AzureSource = "Azure.*"; + /// <summary> + /// Gets or sets the service name for telemetry identification. + /// </summary> public string? ServiceName { get; set; } + /// <summary> + /// Gets or sets the OTLP endpoint URL. + /// </summary> public string? Endpoint { get; set; } // Add documentation for remaining properties
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1349
File: src/Digdir.Domain.Dialogporten.GraphQL/Program.cs:70-74
Timestamp: 2024-11-12T05:32:45.312Z
Learning: In this project, the OpenTelemetry configurations are handled in the `.ConfigureTelemetry()` method.
🔇 Additional comments (2)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
64-68: 🛠️ Refactor suggestion
Replace Console.WriteLine with structured logging
The use of Console.WriteLine for logging configuration details is not recommended for production code. Consider using a bootstrap logger or environment-specific logging configuration.
Consider this approach:
-Console.WriteLine($"[OpenTelemetry] Configuring telemetry for service: {settings.ServiceName}");
-foreach (var attr in settings.ResourceAttributes)
-{
- Console.WriteLine($"[OpenTelemetry] Resource attribute: {attr.Key}={attr.Value}");
-}
+if (builder.Environment.IsDevelopment())
+{
+ Console.WriteLine($"[OpenTelemetry] Configuring telemetry for service: {settings.ServiceName}");
+ foreach (var attr in settings.ResourceAttributes)
+ {
+ Console.WriteLine($"[OpenTelemetry] Resource attribute: {attr.Key}={attr.Value}");
+ }
+}Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
81-112: Same telemetry configuration improvements apply
This code segment has the same duplication issue as identified in the Service/Program.cs file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (3)
57-68: Consider using structured logging instead of Console.WriteLinePrevious attempts to use ILogger failed due to logger initialization issues. Consider creating a dedicated logging abstraction for telemetry configuration that can handle the bootstrap phase.
+ private static class TelemetryLogger + { + public static void Log(string message, params object[] args) => + Console.WriteLine($"[OpenTelemetry] {string.Format(message, args)}"); + } public static WebApplicationBuilder ConfigureTelemetry( this WebApplicationBuilder builder, Action<TelemetrySettings, IConfiguration>? configure = null) { var settings = new TelemetrySettings(); configure?.Invoke(settings, builder.Configuration); - Console.WriteLine($"[OpenTelemetry] Configuring telemetry for service: {settings.ServiceName}"); + TelemetryLogger.Log("Configuring telemetry for service: {0}", settings.ServiceName); foreach (var attr in settings.ResourceAttributes) { - Console.WriteLine($"[OpenTelemetry] Resource attribute: {attr.Key}={attr.Value}"); + TelemetryLogger.Log("Resource attribute: {0}={1}", attr.Key, attr.Value); }
81-94: Improve resource attributes parsing robustnessThe current implementation uses HttpUtility.ParseQueryString which is clever but might not be the most maintainable approach. Consider a more explicit parsing strategy.
- var attributes = System.Web.HttpUtility.ParseQueryString( - resourceAttributes.Replace(',', '&') - ); + var attributes = resourceAttributes + .Split(',') + .Select(pair => pair.Split('=', 2)) + .Where(parts => parts.Length == 2) + .ToDictionary( + parts => parts[0].Trim(), + parts => parts[1].Trim() + );
96-145: Add XML documentation for telemetry configuration optionsThe telemetry configuration is comprehensive but would benefit from XML documentation explaining the available options and their impact.
Consider adding documentation:
+ /// <summary> + /// Configures OpenTelemetry instrumentation with support for both OTLP and Azure Monitor exporters. + /// </summary> + /// <param name="builder">The web application builder.</param> + /// <param name="configure">Optional action to configure telemetry settings.</param> + /// <remarks> + /// Supported environment variables: + /// - OTEL_SERVICE_NAME: Name of the service + /// - OTEL_EXPORTER_OTLP_ENDPOINT: OTLP endpoint URL + /// - OTEL_EXPORTER_OTLP_PROTOCOL: Protocol (grpc, http/protobuf) + /// - OTEL_RESOURCE_ATTRIBUTES: Comma-separated key=value pairs + /// - APPLICATIONINSIGHTS_CONNECTION_STRING: Azure Application Insights connection string + /// </remarks> public static WebApplicationBuilder ConfigureTelemetry( this WebApplicationBuilder builder, Action<TelemetrySettings, IConfiguration>? configure = null)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
Learnt from: knuhau
PR: digdir/dialogporten#1349
File: src/Digdir.Domain.Dialogporten.GraphQL/Program.cs:70-74
Timestamp: 2024-11-12T05:32:45.312Z
Learning: In this project, the OpenTelemetry configurations are handled in the `.ConfigureTelemetry()` method.
🔇 Additional comments (3)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
152-167: LGTM! Well-structured telemetry settings class.
The TelemetrySettings class provides a clean encapsulation of telemetry configuration with appropriate defaults for trace sources.
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
54-85: LGTM! Consistent telemetry configuration
The telemetry configuration follows the same pattern as other services, ensuring consistency across the application.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
81-112: LGTM! Well-integrated telemetry configuration
The telemetry configuration is properly integrated with the existing logging and monitoring infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (2)
109-130: Improve HTTP instrumentation configuration.The current setup of HTTP client instrumentation could be optimized:
- The filter logic is nested inside FilterHttpRequestMessage
- RecordException setting could be shared
Consider this refactoring:
.AddHttpClientInstrumentation(o => { o.RecordException = true; - o.FilterHttpRequestMessage = (_) => + o.FilterHttpRequestMessage = (request) => { var parentActivity = Activity.Current?.Parent; - if (parentActivity != null && parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal)) + return parentActivity?.Source.Name != "Azure.Core.Http"; - { - return false; - } - return true; }; })
152-167: Consider making TelemetrySettings immutable.The TelemetrySettings class could benefit from immutability to prevent accidental modifications after initialization.
Consider this pattern:
- public class TelemetrySettings + public sealed class TelemetrySettings { private const string MassTransitSource = "MassTransit"; private const string AzureSource = "Azure.*"; - public string? ServiceName { get; set; } - public string? Endpoint { get; set; } - public string? Protocol { get; set; } - public string? AppInsightsConnectionString { get; set; } + public string? ServiceName { get; init; } + public string? Endpoint { get; init; } + public string? Protocol { get; init; } + public string? AppInsightsConnectionString { get; init; }src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
81-112: Improve resource attributes parsing and validation.The current implementation of resource attributes parsing could be enhanced:
- The error message could be more descriptive
- The empty value handling could be more explicit
Consider these improvements:
try { var attributes = System.Web.HttpUtility.ParseQueryString( resourceAttributes.Replace(',', '&') ); foreach (string key in attributes.Keys) { - if (!string.IsNullOrEmpty(key)) + if (string.IsNullOrEmpty(key)) + { + continue; + } + var value = attributes[key]; + if (value == null) { - settings.ResourceAttributes[key] = attributes[key] ?? string.Empty; + Log.Warning("Empty value for resource attribute key: {Key}", key); + continue; } + settings.ResourceAttributes[key] = value; } } catch (Exception ex) { throw new InvalidOperationException( - "Failed to parse OTEL_RESOURCE_ATTRIBUTES. Expected format: key1=value1,key2=value2", + "Failed to parse OTEL_RESOURCE_ATTRIBUTES. Expected format: key1=value1,key2=value2. " + + "Values should not be empty and keys should be unique.", ex ); }src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
61-84: Consider enhancing resource attributes parsing robustness.While the current implementation is functional, consider these improvements:
- Add validation for attribute values to ensure they are valid OTEL attributes
- Add debug logging for successful parsing and warnings for skipped entries
- Consider using a more direct parsing approach instead of HttpUtility.ParseQueryString
Here's a suggested implementation:
var resourceAttributes = configuration["OTEL_RESOURCE_ATTRIBUTES"]; if (!string.IsNullOrEmpty(resourceAttributes)) { try { - var attributes = System.Web.HttpUtility.ParseQueryString( - resourceAttributes.Replace(',', '&') - ); - foreach (string key in attributes.Keys) + foreach (var pair in resourceAttributes.Split(',', StringSplitOptions.RemoveEmptyEntries)) { - if (!string.IsNullOrEmpty(key)) + var parts = pair.Split('=', 2); + if (parts.Length != 2) { - settings.ResourceAttributes[key] = attributes[key] ?? string.Empty; + Log.Warning("Skipping invalid attribute pair: {Pair}", pair); + continue; } + + var key = parts[0].Trim(); + var value = parts[1].Trim(); + + if (string.IsNullOrEmpty(key)) + { + Log.Warning("Skipping empty key in pair: {Pair}", pair); + continue; + } + + settings.ResourceAttributes[key] = value; + Log.Debug("Added resource attribute: {Key}={Value}", key, value); } } catch (Exception ex)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1528
File: src/Digdir.Domain.Dialogporten.Service/Program.cs:56-58
Timestamp: 2024-12-10T10:54:37.635Z
Learning: In `src/Digdir.Domain.Dialogporten.Service/Program.cs`, when running locally without Docker, OTEL settings (`OTEL_SERVICE_NAME`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_PROTOCOL`) are optional and may not be provided, so validation for these settings is not required.
🔇 Additional comments (2)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
64-68: 🛠️ Refactor suggestion
Replace Console.WriteLine with proper logging.
Using Console.WriteLine for logging bypasses the logging framework's configuration and features.
Create a temporary logger factory for startup logging:
+ using var loggerFactory = LoggerFactory.Create(builder =>
+ {
+ builder.AddConsole();
+ });
+ var logger = loggerFactory.CreateLogger("TelemetryConfiguration");
- Console.WriteLine($"[OpenTelemetry] Configuring telemetry for service: {settings.ServiceName}");
+ logger.LogInformation("Configuring telemetry for service: {ServiceName}", settings.ServiceName);Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
54-59: LGTM! Configuration aligns with local development requirements.
The telemetry configuration correctly implements optional OTEL settings with appropriate fallbacks, which is suitable for both local development and containerized environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
61-84: Consider adding logging for successful attribute parsing.While error handling is good, adding debug-level logging for successful attribute parsing would help with observability.
try { var attributes = System.Web.HttpUtility.ParseQueryString( resourceAttributes.Replace(',', '&') ); + Log.Debug("Successfully parsed {Count} resource attributes", attributes.Count); foreach (string key in attributes.Keys) {src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
109-130: Consider adding metrics for filtered requests.While the filtering logic is correct, adding metrics to track filtered requests would help monitor the filtering effectiveness.
.AddHttpClientInstrumentation(o => { o.RecordException = true; o.FilterHttpRequestMessage = (_) => { var parentActivity = Activity.Current?.Parent; if (parentActivity != null && parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal)) { + Metrics.CreateCounter<long>("filtered_requests").Add(1, new("source", "azure_core")); return false; } return true; }; })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1528
File: src/Digdir.Domain.Dialogporten.Service/Program.cs:56-58
Timestamp: 2024-12-10T10:54:37.635Z
Learning: In `src/Digdir.Domain.Dialogporten.Service/Program.cs`, when running locally without Docker, OTEL settings (`OTEL_SERVICE_NAME`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_PROTOCOL`) are optional and may not be provided, so validation for these settings is not required.
🔇 Additional comments (5)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
54-58: LGTM! Telemetry configuration setup is well-structured.
The configuration follows best practices by making the settings optional for local development, as confirmed by previous learnings.
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (3)
64-68: Replace Console.WriteLine with proper logging.
Using Console.WriteLine for logging bypasses the logging framework. However, based on past review comments, there's a known issue with logger initialization at this stage.
86-92: LGTM! Protocol handling is robust.
The protocol handling is well-implemented with:
- Case-insensitive comparison
- Support for both 'http' and 'http/protobuf'
- Clear error message for unsupported protocols
152-167: LGTM! Well-structured settings class.
The TelemetrySettings class is well-designed with:
- Clear separation of concerns
- Sensible defaults for trace sources
- Immutable constants for source names
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
81-112: LGTM! Configuration is consistent with other services.
The telemetry configuration follows the established pattern and integrates well with the WebApi's architecture.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (4)
169-184: LGTM! Well-structured telemetry settings class.The
TelemetrySettingsclass provides a clean interface for configuring telemetry with appropriate nullable annotations and default trace sources for MassTransit and Azure services.Consider adding XML documentation for the properties to improve maintainability and IDE support.
71-94: Enhance resource attributes parsing with structured logging.The resource attributes parsing logic is robust, but consider these improvements:
- Replace Console.WriteLine with structured logging
- Add validation for attribute values
Apply this diff:
-Console.WriteLine($"[OpenTelemetry] Resource attribute: {key}={attributes[key]}"); +var logger = builder.Logging.Services.BuildServiceProvider().GetRequiredService<ILogger<TelemetrySettings>>(); +logger.LogInformation("Adding resource attribute: {Key}={Value}", key, attributes[key]); +if (string.IsNullOrWhiteSpace(attributes[key])) +{ + logger.LogWarning("Empty value for resource attribute: {Key}", key); + continue; +}
103-109: Improve protocol validation error message.The protocol validation is correct, but the error message could be more helpful by listing supported protocols.
Apply this diff:
- _ => throw new ArgumentException($"Unsupported protocol: {settings.Protocol}") + _ => throw new ArgumentException( + $"Unsupported protocol: {settings.Protocol}. " + + "Supported protocols are: 'grpc', 'http/protobuf', 'http'")
126-147: Extract filter logic for better maintainability.The instrumentation configuration is comprehensive, but consider extracting the filter logic into named methods for better readability and maintainability.
Apply this diff:
+private static bool ShouldTraceHttpContext(HttpContext context) => + !context.Request.Path.StartsWithSegments("/health"); +private static bool ShouldTraceHttpRequest(HttpRequestMessage _) +{ + var parentActivity = Activity.Current?.Parent; + return parentActivity == null || + !parentActivity.Source.Name.Equals("Azure.Core.Http", StringComparison.Ordinal); +} tracing .AddAspNetCoreInstrumentation(opts => { opts.RecordException = true; - opts.Filter = httpContext => !httpContext.Request.Path.StartsWithSegments("/health"); + opts.Filter = ShouldTraceHttpContext; }) .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 = ShouldTraceHttpRequest; })src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
81-88: Add validation for Application Insights connection string.The telemetry configuration is good, but consider validating the Application Insights connection string format to prevent runtime issues.
Add validation before setting the connection string:
+ var appInsightsConnString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"]; + if (!string.IsNullOrEmpty(appInsightsConnString) && !appInsightsConnString.Contains("InstrumentationKey=")) + { + throw new ArgumentException("Invalid Application Insights connection string format"); + } - settings.AppInsightsConnectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"]; + settings.AppInsightsConnectionString = appInsightsConnString;src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
54-61: Consider centralizing telemetry configurationThe telemetry configuration logic might be duplicated across different Program.cs files (Service, GraphQL, WebApi). Consider moving this configuration to a shared extension method to maintain consistency and reduce duplication.
Example approach:
// In a shared location (e.g., Digdir.Library.Utils.AspNet) public static class TelemetryConfigurationExtensions { public static void ConfigureDialogportenTelemetry( this TelemetrySettings settings, IConfiguration configuration, string defaultServiceName) { settings.ServiceName = configuration["OTEL_SERVICE_NAME"] ?? defaultServiceName; settings.Endpoint = configuration["OTEL_EXPORTER_OTLP_ENDPOINT"]; settings.Protocol = configuration["OTEL_EXPORTER_OTLP_PROTOCOL"]; settings.AppInsightsConnectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"]; // Include the resource attributes parsing logic here } } // Usage in Program.cs builder.ConfigureTelemetry((settings, configuration) => { settings.ConfigureDialogportenTelemetry( configuration, builder.Environment.ApplicationName); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.Service/Program.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Service/Program.cs (1)
Learnt from: arealmaas
PR: digdir/dialogporten#1528
File: src/Digdir.Domain.Dialogporten.Service/Program.cs:56-58
Timestamp: 2024-12-10T10:54:37.635Z
Learning: In `src/Digdir.Domain.Dialogporten.Service/Program.cs`, when running locally without Docker, OTEL settings (`OTEL_SERVICE_NAME`, `OTEL_EXPORTER_OTLP_ENDPOINT`, `OTEL_EXPORTER_OTLP_PROTOCOL`) are optional and may not be provided, so validation for these settings is not required.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesSettings.cs (3)
13-14: Add XML documentation for the class and its propertiesThe TelemetrySettings class and its properties would benefit from XML documentation to explain:
- The purpose of each property
- Expected formats and valid values
- Whether certain properties are required for specific scenarios
Example documentation:
+ /// <summary> + /// Configuration settings for OpenTelemetry integration. + /// </summary> public sealed class TelemetrySettings { // ... constants ... + /// <summary> + /// The name of the service to be used in telemetry data. + /// </summary> public string? ServiceName { get; set; } + /// <summary> + /// The OpenTelemetry collector endpoint URL. + /// </summary> public string? Endpoint { get; set; }Also applies to: 18-22
15-16: Consider refining the Azure trace source patternThe
Azure.*pattern is quite broad and might capture more traces than intended. Consider:
- Being more specific about which Azure services you want to trace
- Documenting the impact of these patterns
- Making the patterns configurable rather than constant
Example implementation:
- private const string AzureSource = "Azure.*"; + private const string AzureStorageSource = "Azure.Storage.*"; + private const string AzureIdentitySource = "Azure.Identity.*"; public HashSet<string> TraceSources { get; set; } = new() { - AzureSource, + AzureStorageSource, + AzureIdentitySource, MassTransitSource };Also applies to: 23-27
18-22: Consider adding required field validationSome properties might be required depending on the configuration scenario. Consider adding validation logic or implementing IValidatable interface.
This could be implemented through:
- Constructor validation
- IValidatable interface
- Fluent validation
- Guard clauses in the configuration extension method
Would you like me to provide an example implementation of any of these approaches?
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (3)
98-111: Add validation for endpoint and protocol settingsWhile the protocol handling is good, consider adding more robust validation:
- Move protocol validation to
TelemetrySettings:public class TelemetrySettings { private string? protocol; public string? Protocol { get => protocol; set => protocol = ValidateProtocol(value); } private static string? ValidateProtocol(string? value) { if (string.IsNullOrEmpty(value)) return value; return value.ToLowerInvariant() switch { "grpc" or "http/protobuf" or "http" => value, _ => throw new ArgumentException($"Unsupported protocol: {value}") }; } }
- Add URI validation:
-telemetryBuilder.UseOtlpExporter(otlpProtocol, new Uri(settings.Endpoint)); +if (!Uri.TryCreate(settings.Endpoint, UriKind.Absolute, out var endpoint)) +{ + throw new ArgumentException("Invalid endpoint URI", nameof(settings.Endpoint)); +} +telemetryBuilder.UseOtlpExporter(otlpProtocol, endpoint);
127-131: Enhance health check endpoint filteringThe current health check filter could be more robust.
Consider using a constant or configuration for health check paths:
-opts.Filter = httpContext => !httpContext.Request.Path.StartsWithSegments("/health"); +private static readonly string[] HealthCheckPaths = new[] { "/health", "/health/startup", "/health/liveness", "/health/readiness", "/health/deep" }; +opts.Filter = httpContext => !HealthCheckPaths.Any(path => httpContext.Request.Path.StartsWithSegments(path, StringComparison.OrdinalIgnoreCase));
157-163: Consider adding more metrics instrumentationThe current metrics configuration is good but could be enhanced.
Consider adding these additional instrumentations for better observability:
metrics.AddRuntimeInstrumentation() .AddAspNetCoreInstrumentation() .AddHttpClientInstrumentation() + .AddEventCountersInstrumentation(c => { + c.AddEventSources("Microsoft.AspNetCore.Hosting"); + c.AddEventSources("Microsoft-AspNetCore-Server-Kestrel"); + }) .AddAzureMonitorMetricExporter();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs(2 hunks)src/Digdir.Library.Utils.AspNet/AspNetUtilitiesSettings.cs(1 hunks)src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj
🔇 Additional comments (4)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesSettings.cs (1)
22-22: Add validation for ResourceAttributes format
The ResourceAttributes property should follow the W3C trace context format. Consider adding validation or documentation about the expected format (e.g., key1=value1,key2=value2).
Let's check if there are any existing validation patterns in the codebase:
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (3)
6-6: LGTM: Imports are well-organized and necessary
The added imports provide all required dependencies for OpenTelemetry integration.
Also applies to: 10-10, 15-19
57-65: Replace Console.WriteLine with structured logging
Using Console.WriteLine for logging is not recommended in production code. Use ILogger instead for better configurability and to respect logging levels.
132-144: Improve HTTP client instrumentation configuration
The current implementation of FilterHttpRequestMessage could be improved.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
arealmaas
force-pushed
the
feat/add-otel-exporter
branch
from
d8b2e3d to
998fc9d
Compare
|
🤖 I have created a release *beep* *boop* --- ## [1.42.0](v1.41.3...v1.42.0) (2024-12-16) ### Features * **apps:** add otel exporter for graphql, service and web-api ([#1528](#1528)) ([cb9238e](cb9238e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
To see your metrics, spin up the OTEL services by running
docker-compose-otel.yml. The service should start sending to the OTLP collector automatically.Example of a trace in the local Jaeger:
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)