feat(janitor): add otlp logger for janitor #1686
Conversation
📝 WalkthroughWalkthroughThis pull request introduces OpenTelemetry integration for the Dialogporten Janitor application. The changes include adding 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Digdir.Domain.Dialogporten.Janitor/Common/Extensions/OpenTelemetryExtensions.cs (3)
31-36: Refactor duplicated protocol parsing logicThe logic for parsing the OTLP protocol is duplicated in both the
AddDialogportenTelemetrymethod (lines 31-36) and theParseOtlpProtocolmethod (lines 137-145). To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider refactoring the code to use theParseOtlpProtocolmethod inAddDialogportenTelemetry.Apply this diff to refactor the code:
--- a/src/Digdir.Domain.Dialogporten.Janitor/Common/Extensions/OpenTelemetryExtensions.cs +++ b/src/Digdir.Domain.Dialogporten.Janitor/Common/Extensions/OpenTelemetryExtensions.cs @@ -31,6 +31,2 @@ - var otlpProtocol = configuration[OtelExporterOtlpProtocol]?.ToLowerInvariant() switch - { - "grpc" => OtlpExportProtocol.Grpc, - "http/protobuf" => OtlpExportProtocol.HttpProtobuf, - _ => throw new ArgumentException($"Unsupported protocol: {configuration[OtelExporterOtlpProtocol]}") - }; + var otlpProtocol = ParseOtlpProtocol(configuration[OtelExporterOtlpProtocol]);Also applies to: 137-145
28-29: Log a warning when OTLP endpoint is invalidWhen the OTLP endpoint is not a well-formed URI, the
AddDialogportenTelemetrymethod exits silently. Consider logging a warning to inform that OpenTelemetry configuration is skipped due to an invalid endpoint. This will aid in diagnosing configuration issues.Apply this diff to add a log message:
+ if (!Uri.IsWellFormedUriString(configuration[OtelExporterOtlpEndpoint], UriKind.Absolute)) + { + services.AddLogging(loggingBuilder => + { + loggingBuilder.AddConsole(); + var logger = loggingBuilder.Services.BuildServiceProvider().GetRequiredService<ILogger<OpenTelemetryExtensions>>(); + logger.LogWarning("Invalid OTLP endpoint: {Endpoint}. OpenTelemetry will not be configured.", configuration[OtelExporterOtlpEndpoint]); + }); + return services; + }
116-135: Ensure consistent configuration handling inTryWriteToOpenTelemetryIn the
TryWriteToOpenTelemetrymethod, consider handling exceptions more gracefully by logging errors instead of swallowing them silently. This will make debugging easier if there are issues with the OTLP configuration.Apply this diff to add logging for exceptions:
try { var protocol = ParseOtlpProtocol(otelProtocol); return config.WriteTo.OpenTelemetry(ConfigureOtlpSink(otelEndpoint, protocol)); } catch (ArgumentException ex) { - return config; + Log.Logger.Warning(ex, "Failed to configure OpenTelemetry logging. Falling back to default logger."); + return config; }src/Digdir.Domain.Dialogporten.Janitor/Program.cs (1)
16-16: Review necessity of Application Insights telemetry configurationEven though the application is transitioning from Application Insights to OpenTelemetry, the
TelemetryConfiguration.CreateDefault()is still being registered (line 62), andMicrosoft.ApplicationInsights.Extensibilityis being imported (line 16). If Application Insights is no longer used, consider removing this registration and the associated using directive to clean up unnecessary dependencies.Apply this diff to remove the unnecessary Application Insights configuration:
-using Microsoft.ApplicationInsights.Extensibility; // Using two-stage initialization to catch startup errors. Log.Logger = new LoggerConfiguration() .MinimumLevel.Warning() @@ -58,7 +57,6 @@ static void BuildAndRun(string[] args) .AddInfrastructure(builder.Configuration, builder.Environment) .WithoutPubSubCapabilities() .Build() - .AddSingleton(TelemetryConfiguration.CreateDefault());Also applies to: 58-62
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.Janitor/Common/Extensions/OpenTelemetryExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj(1 hunks)src/Digdir.Domain.Dialogporten.Janitor/Program.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Dry run deploy apps / Deploy service to test
- GitHub Check: Dry run deploy apps / Deploy graphql to test
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: Dry run deploy apps / Deploy web-api-eu to test
- GitHub Check: build / build-and-test
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Janitor/Program.cs (2)
Line range hint
8-62: Implementation of OpenTelemetry integration is soundThe updates to the logging configuration and service registrations correctly integrate OpenTelemetry into the application. The use of
OpenTelemetryOrConsoleandAddDialogportenTelemetrymethods provides flexibility and aligns with best practices for telemetry in .NET applications.
19-21: Confirm logging configuration consistencyThe logging configurations in both the bootstrap logger (lines 19-21) and the main logger (line 55) use OpenTelemetry sinks. Ensure that this setup does not result in duplicate logs or conflicts between the bootstrap and main logger configurations.
Run the following script to check for duplicate log entries:
Also applies to: 55-55
✅ Verification successful
Bootstrap and main logger configurations are properly implemented
The code ensures no duplicate logging through:
- Proper disposal of bootstrap logger before main logger initialization
- Mutually exclusive sink configuration (either Console or OpenTelemetry)
- Conditional OpenTelemetry setup based on environment configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for duplicate logs when running the application. # Test: Run the application and inspect the logs for duplicates. # Expect: No duplicate log entries. dotnet run --project src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj | tee application.log # Analyze the log file for duplicates awk '{ print $0 }' application.log | sort | uniq -dLength of output: 456
Script:
#!/bin/bash # Find TryWriteToOpenTelemetry implementation ast-grep --pattern 'TryWriteToOpenTelemetry' # Find logger configuration patterns rg -l "LoggerConfiguration" --type cs # Look for logger disposal or reconfiguration rg "CloseAndFlush|Log\.Logger" --type cs -A 2Length of output: 3029
Script:
#!/bin/bash # Get the OpenTelemetry extension implementation from Janitor project cat src/Digdir.Domain.Dialogporten.Janitor/Common/Extensions/OpenTelemetryExtensions.cs # Get the full Program.cs to analyze logger lifecycle cat src/Digdir.Domain.Dialogporten.Janitor/Program.csLength of output: 8374
src/Digdir.Domain.Dialogporten.Janitor/Digdir.Domain.Dialogporten.Janitor.csproj (1)
21-29: New package references enhance telemetry capabilitiesThe addition of OpenTelemetry and related package references is appropriate and effectively expands the application's logging and telemetry functionality.
🤖 I have created a release *beep* *boop* --- ## [1.45.0](v1.44.2...v1.45.0) (2025-01-15) ### Features * added id to attachments, ApiActions and GuiActions in DialogCreate ([#1670](#1670)) ([470e5a9](470e5a9)) * **apps:** export logs to open telemetry endpoint ([#1617](#1617)) ([1a71763](1a71763)) * **janitor:** add otlp logger for janitor ([#1686](#1686)) ([2e1656b](2e1656b)) ### Bug Fixes * **app:** Add missing telemetry setup GraphQL and Service ([#1695](#1695)) ([601a826](601a826)) * Authentication level claim is 0 in dialog token ([#1654](#1654)) ([37e545a](37e545a)) * **graphql:** Add missing activity types ([#1684](#1684)) ([a0697ae](a0697ae)) * **graphql:** Set max execution depth to allow inspection query ([#1679](#1679)) ([6265110](6265110)), closes [#1680](#1680) * **web-api:** re-enable health checks ([#1681](#1681)) ([96c2c3e](96c2c3e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)