chore(web-api): separate the telemetry-configuration into aspnet utils library #1341
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request involves the removal of the 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: 1
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
57-61: Specify a sampling strategy for the production environmentCurrently,
AlwaysOnSampler()is set for the development environment (lines 57~ to 60~), but no sampler is specified for the production environment. This means the default sampler will be used in production, which may not align with your desired telemetry data collection strategy. To control the volume of telemetry data and manage costs, consider specifying an appropriate sampler for production.Here is a suggested addition:
if (builder.Environment.IsDevelopment()) { tracing.SetSampler(new AlwaysOnSampler()); } +else +{ + // Set a sampler for production, e.g., + tracing.SetSampler(new TraceIdRatioBasedSampler(0.1)); // Adjust the sampling rate as needed +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (0 hunks)
- src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (2 hunks)
- src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (1 hunks)
💤 Files with no reviewable changes (2)
- src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/WebApplicationBuilderExtensions.cs
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
🔇 Additional comments (3)
src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj (2)
10-11: LGTM! Package selection aligns with the PR objectives.The addition of these telemetry packages aligns well with the PR's objective of separating telemetry configuration into the ASP.NET utils library. The combination provides comprehensive monitoring capabilities:
- Azure Monitor and Application Insights for cloud monitoring
- OpenTelemetry for standardized telemetry
- PostgreSQL instrumentation for database monitoring
- Runtime instrumentation for system metrics
Also applies to: 15-17
10-11: Verify package versions and compatibility.Let's ensure the selected package versions are appropriate and compatible:
Also applies to: 15-17
✅ Verification successful
Let me try a different approach to verify the package versions and their usage in the codebase.
Let's check the actual usage of these packages to ensure they are properly integrated and not conflicting.
Let's check the content of the dependent projects to ensure there's no duplication or conflicts in telemetry configuration.
Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the telemetry package versions and their integration.
The telemetry package versions are appropriate and well-integrated.
The implementation shows a well-structured approach where:
- The WebApi and Service projects properly utilize the centralized telemetry configuration from Utils.AspNet
- The GraphQL project has a custom ApplicationInsights integration for GraphQL-specific telemetry needs
- There's no duplication of telemetry configuration across projects
- The package versions are used consistently:
- Azure.Monitor.OpenTelemetry.AspNetCore (1.2.0)
- Microsoft.ApplicationInsights.AspNetCore (2.22.0)
- OpenTelemetry packages (1.9.0)
The configuration supports both local development (Application Insights SDK) and production (Azure Monitor) scenarios with proper fallback mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for latest stable versions and known vulnerabilities # Test 1: Check if these are the latest stable versions echo "Checking latest versions from NuGet..." for package in \ "Azure.Monitor.OpenTelemetry.AspNetCore" \ "Microsoft.ApplicationInsights.AspNetCore" \ "Npgsql.OpenTelemetry" \ "OpenTelemetry" \ "OpenTelemetry.Instrumentation.Runtime"; do curl -s "https://api.nuget.org/v3-flatcontainer/$package/index.json" | jq -r '.versions[]' | sort -V | tail -n 1 done # Test 2: Look for version constraints in other project files echo "Checking for version constraints in other projects..." fd -e csproj -x grep -l "OpenTelemetry"Length of output: 2170
Script:
#!/bin/bash # Let's examine the content of the csproj file to understand the full context echo "=== Content of Digdir.Library.Utils.AspNet.csproj ===" cat ./src/Digdir.Library.Utils.AspNet/Digdir.Library.Utils.AspNet.csproj # Let's also check if there are any other projects that might be affected echo -e "\n=== Projects referencing Digdir.Library.Utils.AspNet ===" fd -e csproj -x grep -l "Digdir.Library.Utils.AspNet" # Let's also check for any OpenTelemetry usage in the code echo -e "\n=== Files using OpenTelemetry ====" rg "using.*OpenTelemetry" -lLength of output: 1839
Script:
#!/bin/bash # Let's examine the telemetry configuration in the AspNetUtilitiesExtensions echo "=== Telemetry configuration in AspNetUtilitiesExtensions.cs ===" cat src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs # Let's also check if there are any other telemetry configurations in the dependent projects echo -e "\n=== Telemetry configurations in dependent projects ===" rg -l "ApplicationInsights|OpenTelemetry" ./src/Digdir.Domain.Dialogporten.WebApi/ ./src/Digdir.Domain.Dialogporten.GraphQL/ ./src/Digdir.Domain.Dialogporten.Service/Length of output: 4467
Script:
#!/bin/bash # Check the Program.cs files in dependent projects echo "=== WebApi Program.cs ===" cat ./src/Digdir.Domain.Dialogporten.WebApi/Program.cs echo -e "\n=== GraphQL Program.cs ===" cat ./src/Digdir.Domain.Dialogporten.GraphQL/Program.cs echo -e "\n=== Service Program.cs ===" cat ./src/Digdir.Domain.Dialogporten.Service/Program.cs # Check if there are any other telemetry-related configurations echo -e "\n=== ApplicationInsightEventListener.cs ===" cat ./src/Digdir.Domain.Dialogporten.GraphQL/ApplicationInsightEventListener.csLength of output: 20911
src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs (1)
76-79:⚠️ Potential issueAvoid multiple calls to AddOpenTelemetry() to prevent configuration conflicts
In line 52~,
builder.Services.AddOpenTelemetry()is already called to configure OpenTelemetry services. CallingAddOpenTelemetry()again in line 78~ may lead to configuration conflicts or duplicated telemetry data. To ensure a consistent telemetry setup, consider integratingUseAzureMonitor()into the existing OpenTelemetry configuration without callingAddOpenTelemetry()again.⛔ Skipped due to learnings
Learnt from: knuhau PR: digdir/dialogporten#1202 File: src/Digdir.Domain.Dialogporten.WebApi/Program.cs:159-162 Timestamp: 2024-10-18T09:54:43.808Z Learning: Calling `AddOpenTelemetry()` multiple times is safe and does not cause duplicate service registrations.
|
<!--- Provide a general summary of your changes in the Title above --> ## Description <!--- Describe your changes in detail --> Configures opentelemetry for Service. Copy pasta from WebAPI. Dependent on #1341 ## Related Issue(s) - #1262 ## Verification - [ ] **Your** code builds clean without any errors or warnings - [ ] Manual testing done (required) - [ ] Relevant automated test added (if you find this hard, leave it and we'll help out) ## Documentation - [ ] Documentation is updated (either in `docs`-directory, Altinnpedia or a separate linked PR in [altinn-studio-docs.](https://github.com/Altinn/altinn-studio-docs), if applicable) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced logging and telemetry configuration during application startup. - Introduced a modular approach to configuring telemetry settings. - **Bug Fixes** - Improved error handling during application startup to ensure exceptions are logged appropriately. - **Refactor** - Updated method signature for improved telemetry configuration handling. - Removed direct call to Application Insights, streamlining telemetry integration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
As a step to configure telemetry in service and graphql, separating the configuration into the aspnet-project
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
Chores