Add opentelemetry #67

lizard-boy · 2024-10-19T09:05:35Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Adds opentelemetry-based tracing to server projects. Note: tracing is used exclusively as a system profiler to aid in performance optimizations, not intended to trace usage at all. Please let me know if you find some leaked PII in here!

Code changes

launch.json/tasks.json/launchSettings.json Include Events project in builds, correct the port for events processor self host to be consistent with visual studio build settings and pattern for self-host version of other project (it was previously using the Events port by mistake).
docker-compose: include jaeger as a tool to analyze tracing
api/startup: Add tracing attribute to controller action execution. Turn on middleware telemetry to monitor execution of middlewares
tracingAttribute: Logs tracing even at begin and end of action execution as well as wrapping result processing
entityFramework.csproj: include EF-specific opentelemetry package
DiagnosticsConfig: helper class to configure activity and service information in shared service configuration helpers.
MiddlewareAnalysisDiagnosticsHelper: The idea behind this is to wrap all middleware with trace activities to track their execution time -- much like the controller action wrapping done by the TracingAttribute. It works OK, but the library it relies on, Microsoft.Extensions.MiddlewareAnalysis, is poorly documented and seems to be long-forgotten. Sometimes it seems to randomly miss middlewares, but It certainly catches more than if we didn't have it...
diagnosticsHelpers: static class to add configuration helpers for diagnostics purposes
serviceCollectionExtension: Add openetelemetry and diagnostic event emissions listened to by MiddlewareAnalysisDiagnosticsHelper

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request introduces OpenTelemetry-based tracing to the server projects, focusing on system profiling for performance optimization without tracking user-specific data.

Added Jaeger service in docker-compose.yml for distributed tracing analysis
Implemented TracingAttribute in src/Core/Utilities/TracingAttribute.cs to wrap controller actions and results with tracing activities
Created DiagnosticsConfig in src/SharedWeb/Health/DiagnosticsConfig.cs to configure ActivitySource based on project name
Introduced MiddlewareAnalysisDiagnosticAdapter in src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs for middleware execution tracing
Modified src/Api/Startup.cs to integrate tracing and middleware telemetry in the application pipeline

Use Jaeger for tracing

Add diagnostics for aspdotnet core, middleware, sqlserver, ef core, http clients, and action/result filters for controller actions

greptile-apps

_{11 file(s) reviewed, 14 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T09:06:15Z

src/Api/Startup.cs

@@ -199,6 +199,7 @@ public void ConfigureServices(IServiceCollection services)
        {
            config.Conventions.Add(new ApiExplorerGroupConvention());
            config.Conventions.Add(new PublicApiControllersModelConvention());
+            config.Filters.Add(new TracingAttribute()); // Add tracing to controller actions and results


style: Adding tracing to all controller actions may introduce performance overhead. Consider selective tracing for critical paths only.

greptile-apps · 2024-10-19T09:06:15Z

src/Api/Startup.cs

    {
        IdentityModelEventSource.ShowPII = true;
        app.UseSerilog(env, appLifetime, globalSettings);

+        // Middleware telemetry
+        DiagnosticsHelpers.AddMiddlewareDiagnostics(serviceProvider);


logic: Middleware diagnostics may miss some middleware. Ensure comprehensive coverage or implement fallback monitoring.

greptile-apps · 2024-10-19T09:06:16Z

src/Api/Startup.cs

@@ -217,11 +218,15 @@ public void Configure(
        IWebHostEnvironment env,
        IHostApplicationLifetime appLifetime,
        GlobalSettings globalSettings,
-        ILogger<Startup> logger)
+        ILogger<Startup> logger,
+        IServiceProvider serviceProvider)
    {
        IdentityModelEventSource.ShowPII = true;


logic: Showing PII in logs could lead to security vulnerabilities. Ensure this is disabled in production environments.

greptile-apps · 2024-10-19T09:06:42Z

src/Core/Core.csproj

+    <PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" Version="1.5.1-beta.1" />
+    <PackageReference Include="OpenTelemetry.Instrumentation.Http" Version="1.5.1-beta.1" />
+    <PackageReference Include="OpenTelemetry.Instrumentation.SqlClient" Version="1.5.1-beta.1" />


style: These packages are in beta. Consider using stable versions for production, or document the use of beta packages.

greptile-apps · 2024-10-19T09:07:01Z

src/Core/Utilities/TracingAttribute.cs

+            _actionActivity.AddTag("exception", context.Exception.ToString());
+            _actionActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace);


style: Be cautious about logging full exception details, as it may expose sensitive information. Consider logging only exception type and message

greptile-apps · 2024-10-19T09:08:17Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (_activitySource.StartActivity(name, ActivityKind.Server) is Activity activity)
+        {
+            _activities[name] = activity;
+        }


logic: This could potentially lead to a memory leak if activities are not properly removed. Consider implementing a cleanup mechanism

greptile-apps · 2024-10-19T09:08:17Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        _activities[name].AddTag("exception", exception.ToString());
+        _activities[name].AddTag("exceptionStackTrace", exception.StackTrace);


style: Logging full exception details might expose sensitive information. Consider logging only necessary information

greptile-apps · 2024-10-19T09:08:18Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name))
+        {
+            return;
+        }


logic: The activity might have already been stopped if an exception occurred. Add a check to ensure the activity is still running before stopping it

greptile-apps · 2024-10-19T09:08:19Z

src/SharedWeb/Health/MiddlewareAnalysisDiagnosticAdapter.cs

+        if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name))
+        {
+            return;
+        }
+        _activities[name].Stop();
+    }


style: Consider removing the activity from the dictionary after stopping it to prevent potential memory leaks

greptile-apps · 2024-10-19T09:09:14Z

src/SharedWeb/Utilities/ServiceCollectionExtensions.cs

+        services.AddOpenTelemetry().WithTracing(tracerProviderBuilder =>
+                tracerProviderBuilder
+                    .AddSource(diagnosticsConfig.ActivitySource.Name)
+                    .ConfigureResource(resource => resource
+                        .AddService(diagnosticsConfig.ServiceName))
+                    .AddAspNetCoreInstrumentation()
+                    .AddSqlClientInstrumentation()
+                    .AddEntityFrameworkCoreInstrumentation()
+                    .AddHttpClientInstrumentation()
+                    .AddOtlpExporter());


style: Consider adding sampling to reduce the volume of telemetry data in production

MGibson1 added 2 commits June 26, 2024 14:53

Add opentelemetry to shared web

01ce8d7

Use Jaeger for tracing

Add open telemetry

d1a83cf

Add diagnostics for aspdotnet core, middleware, sqlserver, ef core, http clients, and action/result filters for controller actions

greptile-apps bot reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add opentelemetry #67

Add opentelemetry #67

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

		_actionActivity.AddTag("exception", context.Exception.ToString());
		_actionActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace);

		_activities[name].AddTag("exception", exception.ToString());
		_activities[name].AddTag("exceptionStackTrace", exception.StackTrace);

Add opentelemetry #67

Are you sure you want to change the base?

Add opentelemetry #67

Conversation

lizard-boy commented Oct 19, 2024 • edited by greptile-apps bot Loading

Type of change

Objective

Code changes

Before you submit

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading