-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add opentelemetry #67
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
}); | ||
|
||
services.AddSwagger(globalSettings); | ||
|
@@ -217,11 +218,15 @@ public void Configure( | |
IWebHostEnvironment env, | ||
IHostApplicationLifetime appLifetime, | ||
GlobalSettings globalSettings, | ||
ILogger<Startup> logger) | ||
ILogger<Startup> logger, | ||
IServiceProvider serviceProvider) | ||
{ | ||
IdentityModelEventSource.ShowPII = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Showing PII in logs could lead to security vulnerabilities. Ensure this is disabled in production environments. |
||
app.UseSerilog(env, appLifetime, globalSettings); | ||
|
||
// Middleware telemetry | ||
DiagnosticsHelpers.AddMiddlewareDiagnostics(serviceProvider); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Middleware diagnostics may miss some middleware. Ensure comprehensive coverage or implement fallback monitoring. |
||
|
||
// Add general security headers | ||
app.UseMiddleware<SecurityHeadersMiddleware>(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,13 +35,19 @@ | |
<PackageReference Include="Handlebars.Net" Version="2.1.6" /> | ||
<PackageReference Include="MailKit" Version="4.6.0" /> | ||
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="6.0.31" /> | ||
<PackageReference Include="Microsoft.AspNetCore.MiddlewareAnalysis" Version="6.0.20" /> | ||
<PackageReference Include="Microsoft.Azure.Cosmos" Version="3.39.1" /> | ||
<PackageReference Include="Microsoft.Azure.NotificationHubs" Version="4.2.0" /> | ||
<PackageReference Include="Microsoft.Data.SqlClient" Version="5.2.1" /> | ||
<PackageReference Include="Microsoft.Extensions.Caching.Cosmos" Version="1.6.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.EnvironmentVariables" Version="8.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Configuration.UserSecrets" Version="8.0.0" /> | ||
<PackageReference Include="Microsoft.Extensions.Identity.Stores" Version="6.0.31" /> | ||
<PackageReference Include="OpenTelemetry.Exporter.OpenTelemetryProtocol" Version="1.5.1" /> | ||
<PackageReference Include="OpenTelemetry.Extensions.Hosting" Version="1.5.1" /> | ||
<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" /> | ||
Comment on lines
+48
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: These packages are in beta. Consider using stable versions for production, or document the use of beta packages. |
||
<PackageReference Include="Quartz" Version="3.9.0" /> | ||
<PackageReference Include="SendGrid" Version="9.29.3" /> | ||
<PackageReference Include="Serilog.AspNetCore" Version="8.0.1" /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
#nullable enable | ||
|
||
using System.Diagnostics; | ||
using Microsoft.AspNetCore.Mvc.Filters; | ||
using Microsoft.Extensions.DependencyInjection; | ||
|
||
namespace Bit.Core.Utilities; | ||
|
||
public class TracingAttribute : ActionFilterAttribute | ||
{ | ||
private Activity? _actionActivity; | ||
private Activity? _resultActivity; | ||
|
||
public override void OnActionExecuting(ActionExecutingContext context) | ||
{ | ||
var activitySource = context.HttpContext.RequestServices.GetService<ActivitySource>(); | ||
|
||
if (activitySource == null) | ||
{ | ||
return; | ||
} | ||
|
||
var activityName = $"{context.ActionDescriptor.DisplayName}_action"; | ||
_actionActivity = activitySource.StartActivity(activityName, ActivityKind.Server); | ||
} | ||
|
||
public override void OnActionExecuted(ActionExecutedContext context) | ||
{ | ||
if (_actionActivity == null) | ||
{ | ||
return; | ||
} | ||
|
||
if (context.Exception != null) | ||
{ | ||
_actionActivity.AddTag("exception", context.Exception.ToString()); | ||
_actionActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace); | ||
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Be cautious about logging full exception details, as it may expose sensitive information. Consider logging only exception type and message |
||
} | ||
|
||
_actionActivity.Stop(); | ||
} | ||
|
||
public override void OnResultExecuting(ResultExecutingContext context) | ||
{ | ||
var activitySource = context.HttpContext.RequestServices.GetService<ActivitySource>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: This line duplicates the logic from OnActionExecuting. Consider refactoring to a private method to avoid repetition |
||
|
||
if (activitySource == null) | ||
{ | ||
return; | ||
} | ||
|
||
var activityName = $"{context.ActionDescriptor.DisplayName}_result"; | ||
_resultActivity = activitySource.StartActivity(activityName, ActivityKind.Server); | ||
} | ||
|
||
public override void OnResultExecuted(ResultExecutedContext context) | ||
{ | ||
if (_resultActivity == null) | ||
{ | ||
return; | ||
} | ||
|
||
if (context.Exception != null) | ||
{ | ||
_resultActivity.AddTag("exception", context.Exception.ToString()); | ||
_resultActivity.AddTag("exceptionStackTrace", context.Exception.StackTrace); | ||
} | ||
|
||
_resultActivity.Stop(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
using System.Diagnostics; | ||
using Bit.Core.Settings; | ||
|
||
namespace Bit.SharedWeb.Health; | ||
|
||
public class DiagnosticsConfig | ||
{ | ||
public string ServiceName { get; } | ||
public ActivitySource ActivitySource { get; } | ||
|
||
public static DiagnosticsConfig For(GlobalSettings globalSettings) | ||
{ | ||
return new DiagnosticsConfig(globalSettings); | ||
} | ||
Comment on lines
+11
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider making this method an extension method on GlobalSettings for easier use |
||
|
||
private DiagnosticsConfig(GlobalSettings globalSettings) | ||
{ | ||
ServiceName = globalSettings.ProjectName ?? "Bitwarden"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding a constant for the default service name 'Bitwarden' to improve maintainability |
||
ActivitySource = new ActivitySource(ServiceName); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
#nullable enable | ||
|
||
using System.Collections.Concurrent; | ||
using System.Diagnostics; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.Extensions.DiagnosticAdapter; | ||
|
||
namespace Bit.SharedWeb.Health; | ||
|
||
public class MiddlewareAnalysisDiagnosticAdapter | ||
{ | ||
private readonly ActivitySource _activitySource; | ||
private readonly ConcurrentDictionary<string, Activity> _activities = new(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using ConcurrentDictionary<string, Activity?> to handle potential null activities more explicitly |
||
|
||
public MiddlewareAnalysisDiagnosticAdapter(ActivitySource activitySource) | ||
{ | ||
_activitySource = activitySource; | ||
} | ||
|
||
[DiagnosticName("Microsoft.AspNetCore.MiddlewareAnalysis.MiddlewareStarting")] | ||
public void OnMiddlewareStarting(HttpContext httpContext, string name, Guid instance, long timestamp) | ||
{ | ||
if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware") | ||
{ | ||
return; | ||
} | ||
|
||
if (_activitySource.StartActivity(name, ActivityKind.Server) is Activity activity) | ||
{ | ||
_activities[name] = activity; | ||
} | ||
Comment on lines
+28
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This could potentially lead to a memory leak if activities are not properly removed. Consider implementing a cleanup mechanism |
||
} | ||
|
||
[DiagnosticName("Microsoft.AspNetCore.MiddlewareAnalysis.MiddlewareException")] | ||
public void OnMiddlewareException(Exception exception, HttpContext httpContext, string name, Guid instance, long timestamp, long duration) | ||
{ | ||
if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name)) | ||
{ | ||
return; | ||
} | ||
Comment on lines
+37
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
_activities[name].AddTag("exception", exception.ToString()); | ||
_activities[name].AddTag("exceptionStackTrace", exception.StackTrace); | ||
Comment on lines
+41
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Logging full exception details might expose sensitive information. Consider logging only necessary information |
||
|
||
_activities[name].Stop(); | ||
} | ||
|
||
[DiagnosticName("Microsoft.AspNetCore.MiddlewareAnalysis.MiddlewareFinished")] | ||
public void OnMiddlewareFinished(HttpContext httpContext, string name, Guid instance, long timestamp, long duration) | ||
{ | ||
if (name == "Microsoft.AspNetCore.MiddlewareAnalysis.AnalysisMiddleware" || !_activities.ContainsKey(name)) | ||
{ | ||
return; | ||
} | ||
_activities[name].Stop(); | ||
} | ||
Comment on lines
+50
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider removing the activity from the dictionary after stopping it to prevent potential memory leaks |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
#nullable enable | ||
|
||
using System.Diagnostics; | ||
using Bit.SharedWeb.Health; | ||
using Microsoft.Extensions.DependencyInjection; | ||
|
||
namespace Bit.SharedWeb.Utilities; | ||
|
||
public static class DiagnosticsHelpers | ||
{ | ||
public static void AddMiddlewareDiagnostics(IServiceProvider serviceProvider) | ||
{ | ||
var listener = serviceProvider.GetRequiredService<DiagnosticListener>(); | ||
var activitySource = serviceProvider.GetRequiredService<ActivitySource>(); | ||
|
||
listener.SubscribeWithAdapter(new MiddlewareAnalysisDiagnosticAdapter(activitySource)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ | |
using Bit.Core.Vault.Services; | ||
using Bit.Infrastructure.Dapper; | ||
using Bit.Infrastructure.EntityFramework; | ||
using Bit.SharedWeb.Health; | ||
using DnsClient; | ||
using IdentityModel; | ||
using LaunchDarkly.Sdk.Server; | ||
|
@@ -48,6 +49,7 @@ | |
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.HttpOverrides; | ||
using Microsoft.AspNetCore.Identity; | ||
using Microsoft.AspNetCore.MiddlewareAnalysis; | ||
using Microsoft.AspNetCore.Mvc.Localization; | ||
using Microsoft.Azure.Cosmos.Fluent; | ||
using Microsoft.Extensions.Caching.Cosmos; | ||
|
@@ -57,6 +59,8 @@ | |
using Microsoft.Extensions.Hosting; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.Extensions.Options; | ||
using OpenTelemetry.Resources; | ||
using OpenTelemetry.Trace; | ||
using Serilog.Context; | ||
using StackExchange.Redis; | ||
using NoopRepos = Bit.Core.Repositories.Noop; | ||
|
@@ -236,6 +240,22 @@ public static void AddDefaultServices(this IServiceCollection services, GlobalSe | |
// Required for HTTP calls | ||
services.AddHttpClient(); | ||
|
||
// Add open telemetry | ||
var diagnosticsConfig = DiagnosticsConfig.For(globalSettings); | ||
services.AddSingleton(diagnosticsConfig.ActivitySource); | ||
services.AddMiddlewareAnalysis(); | ||
services.Insert(0, ServiceDescriptor.Transient<IStartupFilter, AnalysisStartupFilter>()); // Insert at beginning to catch all middleware | ||
services.AddOpenTelemetry().WithTracing(tracerProviderBuilder => | ||
tracerProviderBuilder | ||
.AddSource(diagnosticsConfig.ActivitySource.Name) | ||
.ConfigureResource(resource => resource | ||
.AddService(diagnosticsConfig.ServiceName)) | ||
.AddAspNetCoreInstrumentation() | ||
.AddSqlClientInstrumentation() | ||
.AddEntityFrameworkCoreInstrumentation() | ||
.AddHttpClientInstrumentation() | ||
.AddOtlpExporter()); | ||
Comment on lines
+248
to
+257
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding sampling to reduce the volume of telemetry data in production |
||
|
||
services.AddSingleton<IStripeAdapter, StripeAdapter>(); | ||
services.AddSingleton<Braintree.IBraintreeGateway>((serviceProvider) => | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Adding tracing to all controller actions may introduce performance overhead. Consider selective tracing for critical paths only.