feat(graphql): configure opentelemetry #1343
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to 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: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (2)
Line range hint
44-54: Consider implementing proper disposal of TelemetryConfigurationWhile reusing TelemetryConfiguration is efficient, ensure proper cleanup by registering it with the dependency injection container for lifecycle management.
var builder = WebApplication.CreateBuilder(args); +builder.Services.AddSingleton(telemetryConfiguration);
Line range hint
21-69: Consider separating ApplicationInsights and OpenTelemetry concernsThe current implementation focuses on ApplicationInsights, but the PR objective is to configure OpenTelemetry. Consider:
- Using OpenTelemetry's ApplicationInsights exporter instead of direct ApplicationInsights integration
- Implementing separate configuration methods for each telemetry concern
- Following the OpenTelemetry SDK setup best practices
Example OpenTelemetry setup:
builder.Services.AddOpenTelemetry() .WithTracing(builder => builder .AddSource("Dialogporten.GraphQL") .SetResourceBuilder(ResourceBuilder.CreateDefault() .AddService("Dialogporten.GraphQL")) .AddHotChocolateInstrumentation() .AddAspNetCoreInstrumentation() .AddHttpClientInstrumentation() .AddAzureMonitorTraceExporter());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (4 hunks)
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (3)
Line range hint
30-41: LGTM! Robust exception handling implementation.The exception handling properly distinguishes between cancellation and unexpected errors, with appropriate logging and cleanup.
21-28: 🛠️ Refactor suggestionConsider configuring TelemetryConfiguration with application settings
While creating a single TelemetryConfiguration instance is good practice, consider initializing it with application settings instead of using default configuration. This ensures proper instrumentation key and other telemetry settings are applied consistently.
-var telemetryConfiguration = TelemetryConfiguration.CreateDefault(); +var telemetryConfiguration = new TelemetryConfiguration(); +builder.Configuration.GetSection("ApplicationInsights")?.Bind(telemetryConfiguration);Likely invalid or redundant comment.
68-69: Verify OpenTelemetry configuration implementationThe ConfigureTelemetry extension method is called, but we should verify its implementation to ensure proper setup of OpenTelemetry exporters, samplers, and instrumentation.
✅ Verification successful
Let me try a different search pattern to locate the telemetry configuration implementation.
Let me search for additional OpenTelemetry configuration to get a complete picture.
OpenTelemetry configuration is properly implemented
The
ConfigureTelemetryextension method inDigdir.Library.Utils.AspNethas a comprehensive implementation that includes:
- Service name configuration using the application name
- Tracing configuration with:
- Development environment sampling (AlwaysOnSampler)
- ASP.NET Core instrumentation (excluding health endpoints)
- HTTP client instrumentation
- Npgsql (PostgreSQL) instrumentation
- Metrics configuration with runtime instrumentation
- Azure Monitor integration when Application Insights connection string is available
- Fallback to Application Insights SDK for local development
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ConfigureTelemetry implementation rg -t cs "ConfigureTelemetry.*{" -A 10Length of output: 146
Script:
#!/bin/bash # Search for ConfigureTelemetry implementation with a simpler pattern rg -t cs "ConfigureTelemetry" -A 10 # Also search for any telemetry-related configuration files fd -e cs -e json telemetry -iLength of output: 3057
Script:
#!/bin/bash # Search for remaining OpenTelemetry configuration rg -t cs "WithTracing|AddOpenTelemetry" -A 15 # Look for any metrics configuration rg -t cs "WithMetrics" -A 10Length of output: 3923
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (2)
Line range hint
32-44: Add parameter validation for telemetryConfiguration.While the error handling and method signature update look good, consider adding parameter validation to ensure the telemetryConfiguration is not null.
static void BuildAndRun(string[] args, TelemetryConfiguration telemetryConfiguration) { + ArgumentNullException.ThrowIfNull(telemetryConfiguration); var builder = WebApplication.CreateBuilder(args);
Line range hint
21-69: Consider implementing an OpenTelemetry Resource configuration.To enhance the observability setup, consider adding OpenTelemetry Resource attributes to provide better context for your telemetry data. This would help with service identification and environment classification.
Example implementation:
using OpenTelemetry.Resources; var resourceBuilder = ResourceBuilder.CreateDefault() .AddService(serviceName: "Dialogporten.GraphQL", serviceVersion: Assembly.GetExecutingAssembly().GetName().Version?.ToString() ?? "unknown") .AddAttributes(new Dictionary<string, object> { ["deployment.environment"] = builder.Environment.EnvironmentName, ["service.instance.id"] = Environment.MachineName });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (4 hunks)
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (2)
21-27: Well-implemented two-stage initialization pattern!The implementation of a bootstrap logger with a shared TelemetryConfiguration instance is a good practice that ensures:
- Early logging capabilities before the full application starts
- Efficient resource usage by reusing the telemetry configuration
- Proper error capture during startup
54-54: 🛠️ Refactor suggestionVerify OpenTelemetry configuration implementation and consider code organization.
- The ConfigureTelemetry() extension method needs verification to ensure proper OpenTelemetry setup.
- Consider moving the telemetry configuration closer to other observability-related setup (like Serilog configuration) for better code organization.
Let's verify the telemetry configuration implementation:
Consider reorganizing the builder setup to group related configurations:
builder.Host.UseSerilog((context, services, configuration) => configuration .MinimumLevel.Warning() .ReadFrom.Configuration(context.Configuration) .ReadFrom.Services(services) .Enrich.FromLogContext() .WriteTo.Console(formatProvider: CultureInfo.InvariantCulture) .WriteTo.ApplicationInsights(telemetryConfiguration, TelemetryConverter.Traces)); + builder.ConfigureTelemetry(); + builder.Configuration .AddAzureConfiguration(builder.Environment.EnvironmentName) .AddLocalConfiguration(builder.Environment);Also applies to: 68-69
✅ Verification successful
Let me gather more information about the telemetry configuration implementation.
Let me check one more aspect to understand how ApplicationInsights is configured across the application.
Telemetry configuration is properly implemented and well-structured
The verification reveals that:
- The
ConfigureTelemetry()extension method is well-implemented inAspNetUtilitiesExtensions.cswith:
- Proper OpenTelemetry service configuration
- Development-specific sampling
- ASP.NET Core and HTTP client instrumentation
- PostgreSQL tracing
- ApplicationInsights is correctly integrated with Serilog for logging
- The current code organization is actually logical:
- Bootstrap logging is set up first
- Application telemetry is configured after basic builder setup
- This separation is intentional and follows a clear initialization pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ConfigureTelemetry implementation rg -l "ConfigureTelemetry.*WebApplicationBuilder" --type cs # Check for OpenTelemetry setup patterns ast-grep --pattern 'AddOpenTelemetry()'Length of output: 164
Script:
#!/bin/bash # Check the implementation of ConfigureTelemetry in AspNetUtilitiesExtensions.cs rg -A 20 "ConfigureTelemetry.*WebApplicationBuilder" src/Digdir.Library.Utils.AspNet/AspNetUtilitiesExtensions.cs # Look for other telemetry-related configurations rg -l "OpenTelemetry|ApplicationInsights" --type cs # Check for any telemetry setup patterns ast-grep --pattern 'AddTelemetry($$$)' ast-grep --pattern 'Services.Configure<$_>($$$)'Length of output: 1573
Script:
#!/bin/bash # Check ApplicationInsights configuration in GraphQL project rg -A 10 "ApplicationInsights" src/Digdir.Domain.Dialogporten.GraphQL/Program.cs # Check if there are any other telemetry configurations in the GraphQL project rg "ConfigureTelemetry|OpenTelemetry" src/Digdir.Domain.Dialogporten.GraphQL/Program.csLength of output: 1343
🤖 I have created a release *beep* *boop* --- ## [1.27.0](v1.26.3...v1.27.0) (2024-10-29) ### Features * Add restrictions to Transmissions reference hierarchy ([#1310](#1310)) ([e3d53ca](e3d53ca)) * **graphql:** configure opentelemetry ([#1343](#1343)) ([e31c08b](e31c08b)) * **infrastructure:** add availability test for apim ([#1327](#1327)) ([1f9fa2b](1f9fa2b)) * **service:** configure opentelemetry ([#1342](#1342)) ([513d5e4](513d5e4)) * **utils:** configure open telemetry tracing for masstransit in aspnet package ([#1344](#1344)) ([5ec3b84](5ec3b84)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Are Almaas <[email protected]>
Description
Adds opentelemetry for GraphQL
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
Refactor