chore(graphql): Removed Application Insights SDK, added OpenTelemetry #1349
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant changes to the telemetry tracking within the GraphQL application. 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: 3
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (1)
Line range hint
20-86: Documentation and testing needed for telemetry changes.As mentioned in the PR objectives, documentation and tests are missing. Consider:
- Adding XML documentation comments explaining the telemetry setup
- Creating integration tests to verify:
- Proper span creation and propagation
- Correct attribute injection
- Sampling behavior
Would you like help with:
- Generating documentation templates for the telemetry configuration?
- Creating test examples for OpenTelemetry integration?
src/Digdir.Domain.Dialogporten.GraphQL/OpenTelemetryEventListener.cs (1)
35-35: Use standard attribute keyenduser.idfor authenticated userAccording to OpenTelemetry Semantic Conventions, the attribute key for capturing the authenticated user ID is
enduser.id. Using the standard key ensures consistency and better interoperability with OpenTelemetry-compatible tools.Apply this diff to update the attribute key:
- activity.SetTag("user.id", httpContext.User.Identity?.Name ?? "Not authenticated"); + activity.SetTag("enduser.id", httpContext.User.Identity?.Name ?? "Not authenticated");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/Digdir.Domain.Dialogporten.GraphQL/ApplicationInsightEventListener.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/OpenTelemetryEventListener.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (5 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (1 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Domain.Dialogporten.GraphQL/ApplicationInsightEventListener.cs
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (1)
19-19: LGTM! Change aligns with the transition to OpenTelemetry.The replacement of the diagnostic event listener is consistent with the PR objective of migrating from Application Insights to OpenTelemetry.
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (3)
33-33: LGTM! Good practice passing telemetry configuration.The updated method signature properly injects the telemetry configuration, ensuring consistent configuration throughout the application.
86-86: LGTM! Proper service registration.The HttpContextAccessor registration is correctly placed and follows dependency injection best practices.
20-28: Verify the continued use of Application Insights in logging configuration.While transitioning to OpenTelemetry, the code still uses Application Insights for logging. Is this intentional for a gradual transition? If not, consider updating the logging configuration to use OpenTelemetry's logging provider instead.
✅ Verification successful
Application Insights usage is part of an ongoing transition to OpenTelemetry
The continued use of Application Insights in the logging configuration is intentional and correct at this stage because:
- The codebase shows a clear transition pattern where both systems coexist:
- OpenTelemetry is being integrated for tracing with the new
OpenTelemetryEventListener- Application Insights is still handling logging through Serilog across multiple services
- The transition is being handled systematically through
AspNetUtilitiesExtensions.cswhich:
- Configures OpenTelemetry as the primary telemetry system
- Falls back to Application Insights SDK for local development
- Uses Azure Monitor OpenTelemetry exporter when Application Insights connection string is present
This mixed usage is a recommended approach for gradually transitioning between telemetry systems while maintaining observability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Application Insights is still used elsewhere in the codebase rg -l "ApplicationInsights" --type csLength of output: 446
Script:
#!/bin/bash # Let's check the actual usage context of ApplicationInsights in these files for file in $(rg -l "ApplicationInsights" --type cs); do echo "=== $file ===" rg "ApplicationInsights" -B 2 -A 2 "$file" doneLength of output: 6120
Script:
#!/bin/bash # Check for OpenTelemetry usage to understand the transition state rg "OpenTelemetry" -B 2 -A 2 --type csLength of output: 3986
Script:
#!/bin/bash # Check if there are any OpenTelemetry logging providers configured ast-grep --pattern 'AddOpenTelemetry()$$$'Length of output: 44
|
Description
Removed Application Insights SDK, added OpenTelemetry
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
OpenTelemetryEventListenerfor enhanced tracing and monitoring of GraphQL requests.Bug Fixes
ApplicationInsightEventListener, improving the overall telemetry approach.Refactor