chore(webapi): Add Opentelemetry tracing and metrics to webapi #1202
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to monitoring and instrumentation within the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (5)
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
|
knuhau
changed the title
# Conflicts: # src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj # src/Digdir.Domain.Dialogporten.WebApi/Program.cs
# Conflicts: # src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
# Conflicts: # src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs # src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj # src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (1)
99-104: Improved Redis configuration with async supportThe changes to the Redis configuration are a step in the right direction. Using
ConnectionMultiplexerFactoryallows for more flexible and potentially asynchronous connection establishment. However, there's room for further improvement.Consider modifying the
ConnectionMultiplexerFactoryto use a truly asynchronous connection method:services.AddStackExchangeRedisCache(options => - options.ConnectionMultiplexerFactory = () => Task.FromResult<IConnectionMultiplexer>( - ConnectionMultiplexer.Connect(infrastructureSettings.Redis.ConnectionString) - )); + options.ConnectionMultiplexerFactory = () => ConnectionMultiplexer.ConnectAsync(infrastructureSettings.Redis.ConnectionString));This change would fully leverage the asynchronous capabilities, potentially improving performance and resource utilization, especially during application startup.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (2)
Line range hint
60-65: Potential duplication of telemetry dataUsing Serilog's Application Insights sink alongside OpenTelemetry's Azure Monitor exporter may send duplicate logs to Application Insights. Consider consolidating logging to use OpenTelemetry exporters exclusively.
139-142: Adjust sampling strategy in development environmentUsing
AlwaysOnSampler()collects all traces, which can be verbose. Consider usingTraceIdRatioBasedSamplerwith a configurable fraction to control trace volume.Apply this change to adjust the sampler:
if (builder.Environment.IsDevelopment()) { - tracing.SetSampler(new AlwaysOnSampler()); + tracing.SetSampler(new TraceIdRatioBasedSampler(0.5)); // Adjust the fraction as needed }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (2 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (5 hunks)
- tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (5)
11-11: LGTM: Azure.Monitor.OpenTelemetry.AspNetCore package additionThe addition of this package is appropriate for integrating Azure Monitor with OpenTelemetry in your ASP.NET Core application. It aligns well with the PR objective of transitioning from Application Insights to OpenTelemetry for tracing.
20-24: LGTM: Core OpenTelemetry packages additionThe addition of these core OpenTelemetry packages is excellent and aligns perfectly with the PR objective. Here's a breakdown of their purposes:
OpenTelemetry: Provides the core OpenTelemetry functionality.OpenTelemetry.Extensions.Hosting: Integrates OpenTelemetry with .NET hosting.OpenTelemetry.Instrumentation.AspNetCore: Adds instrumentation for ASP.NET Core.OpenTelemetry.Instrumentation.Http: Provides instrumentation for HTTP clients.OpenTelemetry.Instrumentation.Runtime: Adds runtime metrics instrumentation.The consistent version (1.9.0) across all packages ensures compatibility. These additions will significantly enhance your application's telemetry capabilities.
25-25: Caution: Beta package and Redis usage verificationThe addition of
OpenTelemetry.Instrumentation.StackExchangeRedisis appropriate if your application uses Redis and you want to include Redis operations in your telemetry data. However, there are two points to consider:
This package is currently in beta (version 1.9.0-beta.1). Using beta versions in a production environment may introduce instability. Consider if this risk is acceptable for your use case.
If your application doesn't use Redis, this package may be unnecessary.
Please confirm that the application uses Redis and that you're comfortable with using a beta package. You can verify Redis usage with the following script:
#!/bin/bash # Search for StackExchange.Redis usage in the codebase rg --type csharp 'using StackExchange.Redis' -l rg --type csharp 'IConnectionMultiplexer' -lIf Redis is not used, consider removing this package. If Redis is used but you prefer stability, you might want to wait for a stable release before including this instrumentation.
11-25: Overall assessment: OpenTelemetry integration looks good, with some considerationsThe changes to the project file effectively introduce OpenTelemetry integration, aligning well with the PR objective of replacing Application Insights with OpenTelemetry. The additions cover various aspects of telemetry, including core functionality, ASP.NET Core instrumentation, HTTP client instrumentation, and potential database (PostgreSQL) and cache (Redis) integrations.
Considerations and next steps:
- Verify the necessity of PostgreSQL and Redis instrumentations based on your application's dependencies.
- Assess the risk of using the beta version of the Redis instrumentation package in your environment.
- Review and potentially remove or update existing Application Insights related packages that may no longer be needed.
- Ensure that the
Program.csand other relevant files are updated to utilize these new packages effectively.- Update the project documentation to reflect the transition from Application Insights to OpenTelemetry.
- Plan for comprehensive testing to ensure the new telemetry setup captures all necessary data without performance issues.
To help with the cleanup of potentially obsolete packages, run the following script:
#!/bin/bash # Search for Application Insights usage in the codebase rg --type csharp 'using Microsoft.ApplicationInsights' -lIf this search returns no results, consider removing the
Microsoft.ApplicationInsights.AspNetCorepackage from your project file.
19-19: LGTM: Npgsql.OpenTelemetry package additionThe addition of this package is appropriate for integrating OpenTelemetry with Npgsql, which is beneficial if your application uses PostgreSQL. It will allow you to include database operations in your telemetry data.
Please confirm that the application indeed uses PostgreSQL. If not, this package may be unnecessary. You can verify PostgreSQL usage with the following script:
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (2)
38-38: LGTM: Removed trailing whitespace.The removal of the trailing space in this comment line is a good practice. It helps maintain clean code and reduces unnecessary differences in version control systems. The comment itself provides valuable context about the alphabetical ordering of properties in the Swagger JSON, which is crucial for maintaining deterministic test results across different platforms.
Line range hint
1-93: Overall code quality is high.The
SwaggerSnapshotTestsclass is well-structured and thoroughly commented. The test methodFailIfSwaggerSnapshotDoesNotMatcheffectively ensures that changes to the Swagger JSON are detected and verified. The use ofSortJsonandSortElementmethods to create a deterministic ordering of JSON properties is a clever solution to the cross-platform inconsistency issue.The code demonstrates good practices such as:
- Clear and informative comments
- Effective use of snapshot testing
- Handling of platform-specific inconsistencies
- Proper use of asynchronous programming
No significant issues or areas for improvement are apparent in this file.
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (3)
44-44: Passing TelemetryConfiguration enhances configurabilityPassing
telemetryConfigurationto theBuildAndRunmethod improves configurability and decouples the telemetry setup from the method implementation.Also applies to: 56-56
133-158: OpenTelemetry setup is correctly implementedThe configuration for OpenTelemetry tracing and metrics appears comprehensive, including instrumentation for ASP.NET Core, HTTP clients, PostgreSQL, and Redis. Filtering out health check requests is a good practice to reduce unnecessary data collection.
131-131: Redundant call to AddAuthorizationThe call to
.AddAuthorization()may be redundant ifAddDialogportenAuthenticationalready adds authorization services. Verify if it's necessary to call both methods.To confirm, run the following script to search for
AddAuthorizationcalls:✅ Verification successful
AddAuthorization is not redundant
The
AddDialogportenAuthenticationmethod does not call.AddAuthorization(), so the explicit call to.AddAuthorization()is necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of AddAuthorization in the codebase. # Test: Search for AddAuthorization method calls. Expect: Ensure it's not called redundantly. rg --type cs "\.AddAuthorization\(" -A 2Length of output: 823
Script:
#!/bin/bash # Description: Find the definition of AddDialogportenAuthentication and check if it calls AddAuthorization. # Search for the method definition rg 'AddDialogportenAuthentication' --type cs -A 10 # Additionally, check if AddAuthorization is called within the AddDialogportenAuthentication method ast-grep --lang csharp --pattern $'public static IServiceCollection AddDialogportenAuthentication($_) { $$$ }' | rg '\.AddAuthorization\('Length of output: 5181
Script:
#!/bin/bash # Description: Verify if AddDialogportenAuthentication calls AddAuthorization within its implementation. # Step 1: Find the files where AddDialogportenAuthentication is defined FILES=$(rg --type cs 'public\s+static\s+IServiceCollection\s+AddDialogportenAuthentication' --files-with-matches) # Step 2: For each file, check if AddAuthorization is called within the AddDialogportenAuthentication method for file in $FILES; do echo "Checking in $file" # Extract the method body METHOD_BODY=$(rg -A 20 'public\s+static\s+IServiceCollection\s+AddDialogportenAuthentication' "$file") # Check if AddAuthorization is called within the method body echo "$METHOD_BODY" | rg '\.AddAuthorization\(' doneLength of output: 4279
src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
Outdated
Show resolved
Hide resolved
knuhau
changed the title
knuhau
changed the title
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs (2)
20-20: Review Sampler Choice for Development EnvironmentThe
AlwaysOnSampleris used in development. While this captures all traces, it may lead to large amounts of data. Consider usingAlwaysOffSamplerto disable tracing, orTraceIdRatioBasedSamplerto sample a percentage of requests.Would you like assistance in configuring a different sampler for the development environment?
13-36: Consider Externalizing Telemetry SettingsFor better flexibility and adherence to the configuration-as-code principle, consider moving telemetry settings to
appsettings.jsonor environment variables. This allows changes without modifying code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs (6 hunks)
🧰 Additional context used
🔇 Additional comments (7)
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs (1)
38-46: Verify Application Insights and Azure Monitor ConfigurationThe code uses
UseAzureMonitor()whenAPPLICATIONINSIGHTS_CONNECTION_STRINGis set, andAddApplicationInsightsTelemetry()otherwise. Ensure this logic aligns with your deployment environments and that there are no conflicts between Application Insights SDK and OpenTelemetry.Run this script to check for other telemetry configurations:
✅ Verification successful
Telemetry Configuration Verified Successfully
The telemetry setup correctly uses
UseAzureMonitor()with OpenTelemetry whenAPPLICATIONINSIGHTS_CONNECTION_STRINGis set and falls back toAddApplicationInsightsTelemetry()for local development. No conflicting telemetry configurations were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for additional telemetry configurations # Expected Result: Instances of telemetry configuration without conflicts rg --type cs 'AddApplicationInsightsTelemetry|UseAzureMonitor' -A 5 -B 5Length of output: 5238
Script:
#!/bin/bash # Description: Search for files that configure both OpenTelemetry/AzureMonitor and ApplicationInsightsTelemetry # Expected Result: List of files with potential conflicting telemetry configurations rg --type cs '(AddOpenTelemetry|UseAzureMonitor)' | while read -r file; do if rg --type cs 'AddApplicationInsightsTelemetry' "$file" > /dev/null; then echo "$file" fi doneLength of output: 860
Script:
#!/bin/bash # Description: Search for files that configure both OpenTelemetry/AzureMonitor and ApplicationInsightsTelemetry # Expected Result: List of files with potential conflicting telemetry configurations # Find all .cs files containing AddOpenTelemetry or UseAzureMonitor telemetry_files=$(rg -l --type cs 'AddOpenTelemetry|UseAzureMonitor') # Iterate through each file and check for AddApplicationInsightsTelemetry for file in $telemetry_files; do if rg -q 'AddApplicationInsightsTelemetry' "$file"; then echo "$file" fi doneLength of output: 513
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (6)
10-10: Namespace inclusion is appropriateThe addition of
Digdir.Domain.Dialogporten.WebApi.Common.Extensionsis necessary for accessing extension methods likeConfigureTelemetry().
29-34: Previous comment still appliesThe suggestion to align the logging configuration with OpenTelemetry remains valid.
39-39: PassingTelemetryConfigurationtoBuildAndRunPassing the
TelemetryConfigurationinstance toBuildAndRunensures consistent telemetry configuration throughout the application.
51-51: Method signature updated appropriatelyThe
BuildAndRunmethod now acceptsTelemetryConfiguration, facilitating consistent telemetry setup.
60-60: UsingTelemetryConfigurationin Serilog setupThe logger configuration correctly utilizes the passed
telemetryConfigurationfor Application Insights.
74-74: Configuring telemetry withConfigureTelemetry()The call to
builder.ConfigureTelemetry();integrates the OpenTelemetry setup into the application startup process.
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs
Show resolved
Hide resolved
|
Description
Replace Application Insights tracing with Opentelemetry in the webapi
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes