Refactor caching of components based on options

src/Elastic.OpenTelemetry/Core/ElasticOpenTelemetry.cs

@@ -14,29 +14,18 @@ namespace Elastic.OpenTelemetry.Core;
 
 internal static class ElasticOpenTelemetry
 {
-	private static volatile ElasticOpenTelemetryComponents? SharedComponents;
 	private static int BootstrapCounter;
 
 	public static SdkActivationMethod ActivationMethod;
 
-#pragma warning disable IDE0028 // Simplify collection initialization
-	internal static readonly ConditionalWeakTable<object, BuilderState> BuilderStateTable = new();
-#pragma warning restore IDE0028 // Simplify collection initialization
 
-	private static readonly Lock Lock = new();
+	internal static readonly HashSet<ElasticOpenTelemetryComponents> SharedComponents = [];
 
-	internal static ElasticOpenTelemetryComponents Bootstrap() =>
-		Bootstrap(SdkActivationMethod.NuGet, CompositeElasticOpenTelemetryOptions.DefaultOptions, null);
+	private static readonly Lock Lock = new();
 
 	internal static ElasticOpenTelemetryComponents Bootstrap(SdkActivationMethod activationMethod) =>
 		Bootstrap(activationMethod, CompositeElasticOpenTelemetryOptions.DefaultOptions, null);
 
-	internal static ElasticOpenTelemetryComponents Bootstrap(CompositeElasticOpenTelemetryOptions options) =>
-		Bootstrap(SdkActivationMethod.NuGet, options, null);
-
-	internal static ElasticOpenTelemetryComponents Bootstrap(IServiceCollection? services) =>
-		Bootstrap(SdkActivationMethod.NuGet, CompositeElasticOpenTelemetryOptions.DefaultOptions, services);
-
 	internal static ElasticOpenTelemetryComponents Bootstrap(CompositeElasticOpenTelemetryOptions options, IServiceCollection? services) =>
 		Bootstrap(SdkActivationMethod.NuGet, options, services);
 
@@ -59,79 +48,43 @@ internal static ElasticOpenTelemetryComponents Bootstrap(
 
 		var invocationCount = Interlocked.Increment(ref BootstrapCounter);
 
-		// If an IServiceCollection is provided, we attempt to access any existing
-		// components to reuse them.
-		if (TryGetExistingComponentsFromServiceCollection(services, out var existingComponents))
-		{
-			existingComponents.Logger.LogBootstrapInvoked(invocationCount);
-			return existingComponents;
-		}
-
-		// If an IServiceCollection is provided, but it doesn't yet include any components then
-		// create new components.
-		if (services is not null)
+		// Strictly speaking, we probably don't require locking here as the registration of
+		// OpenTelemetry is expected to run sequentially. That said, the overhead is low
+		// since this is called infrequently.
+		using (var scope = Lock.EnterScope())
 		{
-			// Strictly speaking, we probably don't require locking here as the registration of
-			// OpenTelemetry is expected to run sequentially. That said, the overhead is low
-			// since this is called infrequently.
-			using (var scope = Lock.EnterScope())
+			// If an IServiceCollection is provided, we attempt to access any existing
+			// components to reuse them before accessing any potential shared components.
+			if (services is not null)
 			{
-				// Double-check that no components were created in a race situation.
-				if (TryGetExistingComponentsFromServiceCollection(services, out existingComponents))
+				if (TryGetExistingComponentsFromServiceCollection(services, out var existingComponents))
 				{
 					existingComponents.Logger.LogBootstrapInvoked(invocationCount);
 					return existingComponents;
 				}
-
-				// We don't have components assigned for this IServiceCollection, attempt to use
-				// the existing SharedComponents, or create components.
-				if (TryGetSharedComponents(SharedComponents, stackTrace, out var sharedComponents)
-					&& sharedComponents.Options.Equals(options))
-				{
-					components = sharedComponents;
-				}
-				else
-				{
-					components = SharedComponents = CreateComponents(activationMethod, options, stackTrace);
-				}
-
-				components.Logger.LogBootstrapInvoked(invocationCount);
 			}
 
-			services.AddSingleton(components);
-			services.AddHostedService<ElasticOpenTelemetryService>();
-
-			return components;
-		}
-
-		// When no IServiceCollection is provided we attempt to avoid bootstrapping more than
-		// once. The first call into Bootstrap wins and thereafter the same components are reused.
-		if (TryGetSharedComponents(SharedComponents, stackTrace, out var shared))
-		{
-			// We compare whether the options (values) equal those from the shared components. If the
-			// values of the options differ, we will not reuse the shared components.
-			if (shared.Options.Equals(options))
+			// We don't have components assigned for this IServiceCollection, attempt to use
+			// the existing SharedComponents, or create components.
+			if (TryGetSharedComponents(options, out var sharedComponents))
 			{
-				components = shared;
-				components.Logger.LogSharedComponentsReused();
-				return components;
+				components = sharedComponents;
+			}
+			else
+			{
+				components = CreateComponents(activationMethod, options, stackTrace);
+				components.Logger.LogSharedComponentsNotReused();
+				SharedComponents.Add(components);
 			}
 
-			components = CreateComponents(activationMethod, options, stackTrace);
-			components.Logger.LogSharedComponentsNotReused();
-			return components;
-		}
+			components.Logger.LogBootstrapInvoked(invocationCount);
 
-		using (var scope = Lock.EnterScope())
-		{
-			if (TryGetSharedComponents(SharedComponents, stackTrace, out shared) && shared.Options.Equals(options))
+			if (services is not null)
 			{
-				components = shared;
-				return components;
+				services.AddSingleton(components);
+				services.AddHostedService<ElasticOpenTelemetryService>();
 			}
 
-			// If we get this far, we've been unable to get the shared components
-			components = SharedComponents = CreateComponents(activationMethod, options, stackTrace);
 			return components;
 		}
 
@@ -148,17 +101,22 @@ static bool TryGetExistingComponentsFromServiceCollection(IServiceCollection? se
 			return true;
 		}
 
-		static bool TryGetSharedComponents(ElasticOpenTelemetryComponents? components, StackTrace stackTrace,
+		static bool TryGetSharedComponents(CompositeElasticOpenTelemetryOptions options,
 			[NotNullWhen(true)] out ElasticOpenTelemetryComponents? sharedComponents)
 		{
 			sharedComponents = null;
 
-			if (components is null)
-				return false;
-
-			sharedComponents = components;
+			foreach (var cachedComponents in SharedComponents)
+			{
+				if (cachedComponents.Options.Equals(options))
+				{
+					sharedComponents = cachedComponents;
+					cachedComponents.Logger.LogSharedComponentsReused();
+					return true;
+				}
+			}
 
-			return true;
+			return false;
 		}
 
 		static ElasticOpenTelemetryComponents CreateComponents(
@@ -177,8 +135,8 @@ static ElasticOpenTelemetryComponents CreateComponents(
 		}
 	}
 
-	/// <summary>
-	/// Used for testing.
-	/// </summary>
-	internal static void ResetSharedComponentsForTesting() => SharedComponents = null;
+	///// <summary>
+	///// Used for testing.
+	///// </summary>
+	//internal static void ResetSharedComponentsForTesting() => SharedComponents = null;
 }

src/Elastic.OpenTelemetry/Core/SignalBuilder.cs

@@ -4,6 +4,7 @@
 
 using System.Diagnostics.CodeAnalysis;
 using System.Reflection;
+using System.Runtime.CompilerServices;
 using Elastic.OpenTelemetry.Configuration;
 using Elastic.OpenTelemetry.Diagnostics;
 using Elastic.OpenTelemetry.Instrumentation;
@@ -19,34 +20,38 @@ namespace Elastic.OpenTelemetry.Core;
 /// </summary>
 internal static class SignalBuilder
 {
+#pragma warning disable IDE0028 // Simplify collection initialization
+	private static readonly ConditionalWeakTable<object, BuilderState> BuilderStateTable = new();
+#pragma warning restore IDE0028 // Simplify collection initialization
+
 	private static readonly Lock Lock = new();
 
 	/// <summary>
 	/// Returns the most relevant <see cref="ILogger"/> for builder extension methods to use.
 	/// </summary>
-	/// <param name="components"></param>
-	/// <param name="options"></param>
-	/// <returns></returns>
-	public static ILogger GetLogger(ElasticOpenTelemetryComponents? components, CompositeElasticOpenTelemetryOptions? options) =>
-		components?.Logger ?? options?.AdditionalLogger ?? NullLogger.Instance;
-
-	public static string GetBuilderIdentifier<T>(this T builder) where T : class =>
-		!ElasticOpenTelemetry.BuilderStateTable.TryGetValue(builder, out var state)
-			? builder.GetHashCode().ToString()
-			: $"{state.InstanceIdentifier}:{builder.GetHashCode()}";
-
-	public static void IncrementAndVerifyCallCount(string providerBuilderName, ILogger logger, BuilderState builderState)
+	public static ILogger GetLogger(
+		ElasticOpenTelemetryComponents? components,
+		CompositeElasticOpenTelemetryOptions? options) =>
+			components?.Logger ?? options?.AdditionalLogger ?? NullLogger.Instance;
+
+	/// <summary>
+	/// Returns the most relevant <see cref="ILogger"/> for builder extension methods to use.
+	/// </summary>
+	public static ILogger GetLogger<T>(
+		T builder,
+		ElasticOpenTelemetryComponents? components,
+		CompositeElasticOpenTelemetryOptions? options,
+		BuilderState? builderState) where T : class
 	{
-		// This allows us to track the number of times a specific instance of a builder is configured.
-		// We expect each builder to be configured at most once and log a warning if multiple invocations
-		// are detected.
-		builderState.IncrementWithElasticDefaults();
-
-		if (builderState.WithElasticDefaultsCounter > 1)
-			logger.LogWarning("The `WithElasticDefaults` method has been called {WithElasticDefaultsCount} " +
-				"times on the same `{BuilderType}` (instance: {BuilderInstanceId}). This method is " +
-				"expected to be invoked a maximum of one time.", builderState.WithElasticDefaultsCounter, providerBuilderName,
-				builderState.InstanceIdentifier);
+		if (builderState is not null)
+			return builderState.Components.Logger;
+
+		var logger = components?.Logger ?? options?.AdditionalLogger ?? NullLogger.Instance;
+
+		if (BuilderStateTable.TryGetValue(builder, out builderState))
+			logger = builderState.Components.Logger;
+
+		return logger;
 	}
 
 	public static T WithElasticDefaults<T>(
@@ -60,18 +65,30 @@ public static T WithElasticDefaults<T>(
 		var providerBuilderName = builder.GetType().Name;
 		var logger = GetLogger(components, options);
 
+		BuilderState? builderState = null;
+
 		try
 		{
+			var builderInstanceId = "<unknown>";
+
+			if (BuilderStateTable.TryGetValue(builder, out var existingBuilderState))
+			{
+				builderState = existingBuilderState;
+				builderInstanceId = existingBuilderState.InstanceIdentifier;
+			}
+
+			if (builderState is not null)
+				logger = builderState.Components.Logger;
+
 			// If the signal is disabled via configuration we skip any potential bootstrapping.
 			var configuredSignals = components?.Options.Signals ?? options?.Signals ?? Signals.All;
 			if (!configuredSignals.HasFlagFast(signal))
 			{
-				var builderInstanceId = ElasticOpenTelemetry.BuilderStateTable.TryGetValue(builder, out var builderState) ? builderState.InstanceIdentifier : "<unknown>";
 				logger.LogSignalDisabled(signal.ToString().ToLower(), providerBuilderName, builderInstanceId);
 				return builder;
 			}
 
-			WithElasticDefaults(builder, options, components, services, configure);
+			return WithElasticDefaults(builder, options, components, services, configure);
 		}
 		catch (Exception ex)
 		{
@@ -90,39 +107,56 @@ public static T WithElasticDefaults<T>(
 		IServiceCollection? services,
 		Action<T, BuilderState, IServiceCollection?> configure) where T : class
 	{
-		var builderName = builder.GetType().Name;
+		var providerBuilderName = builder.GetType().Name;
 		var logger = GetLogger(components, options);
 
-		BuilderState builderState;
-
-		// This should not be a hot path, so locking here is fine.
-		using (var scope = Lock.EnterScope())
+		try
 		{
-			if (ElasticOpenTelemetry.BuilderStateTable.TryGetValue(builder, out builderState!))
+			if (BuilderStateTable.TryGetValue(builder, out var builderState))
 			{
-				logger.LogBuilderAlreadyConfigured(builderName, builderState.InstanceIdentifier);
-				IncrementAndVerifyCallCount(builderName, logger, builderState);
+				builderState.Components.Logger.LogBuilderAlreadyConfigured(providerBuilderName, builderState.InstanceIdentifier);
+
+				// This allows us to track the number of times a specific instance of a builder is configured.
+				// We expect each builder to be configured at most once and log a warning if multiple invocations
+				// are detected.
+				builderState.IncrementWithElasticDefaults();
+
+				if (builderState.WithElasticDefaultsCounter > 1)
+					builderState.Components.Logger.LogWarning("The `WithElasticDefaults` method has been called {WithElasticDefaultsCount} " +
+						"times on the same `{BuilderType}` (instance: {BuilderInstanceId}). This method is " +
+						"expected to be invoked a maximum of one time.", builderState.WithElasticDefaultsCounter, providerBuilderName,
+						builderState.InstanceIdentifier);
+
 				return builder;
 			}
 
-			var instanceId = Guid.NewGuid().ToString(); // Used in logging to track duplicate calls to the same builder
+			// This should not be a hot path, so locking here is reasonable.
+			using (var scope = Lock.EnterScope())
+			{
+				var instanceId = Guid.NewGuid().ToString(); // Used in logging to track duplicate calls to the same builder
 
-			// We can't log to the file here as we don't yet have any bootstrapped components.
-			// Therefore, this message will only appear if the consumer provides an additional logger.
-			// This is fine as it's a trace level message for advanced debugging.
-			logger.LogNoExistingComponents(builderName, instanceId);
+				// We can't log to the file here as we don't yet have any bootstrapped components.
+				// Therefore, this message will only appear if the consumer provides an additional logger.
+				// This is fine as it's a trace level message for advanced debugging.
+				logger.LogNoExistingComponents(providerBuilderName, instanceId);
 
-			options ??= CompositeElasticOpenTelemetryOptions.DefaultOptions;
+				options ??= CompositeElasticOpenTelemetryOptions.DefaultOptions;
+				components = ElasticOpenTelemetry.Bootstrap(options, services);
+				builderState = new BuilderState(components, instanceId);
 
-			components = ElasticOpenTelemetry.Bootstrap(options, services);
-			builderState = new BuilderState(components, instanceId);
+				components.Logger.LogStoringBuilderState(providerBuilderName, instanceId);
 
-			components.Logger.LogStoringBuilderState(builderName, builderState.InstanceIdentifier);
+				BuilderStateTable.Add(builder, builderState);
+			}
 
-			ElasticOpenTelemetry.BuilderStateTable.Add(builder, builderState);
+			configure(builder, builderState, services);
+		}
+		catch (Exception ex)
+		{
+			logger.LogError(new EventId(502, "BuilderDefaultsFailed"), ex, "Failed to fully register EDOT .NET " +
+				"defaults on the {ProviderBuilderName}.", builder.GetType().Name);
 		}
 
-		configure(builder, builderState, services);
 		return builder;
 	}
 
@@ -196,7 +230,7 @@ public static void AddInstrumentationViaReflection<T>(T builder, ILogger logger,
 				}
 				catch (Exception ex)
 				{
-					logger.LogError(new EventId(502, "DynamicInstrumentaionFailed"), ex, "Failed to dynamically enable " +
+					logger.LogError(new EventId(503, "DynamicInstrumentaionFailed"), ex, "Failed to dynamically enable " +
 						"{InstrumentationName} on {Provider}.", assemblyInfo.Name, builderTypeName);
 				}
 			}

src/Elastic.OpenTelemetry/Extensions/LoggingProviderBuilderExtensions.cs

@@ -125,20 +125,17 @@ internal static LoggerProviderBuilder WithElasticDefaultsCore(
 		ElasticOpenTelemetryComponents? components,
 		IServiceCollection? services)
 	{
-		var callCount = Interlocked.Increment(ref WithElasticDefaultsCallCount);
-
-		var logger = components?.Logger ?? options?.AdditionalLogger;
+		var logger = SignalBuilder.GetLogger(builder, components, options, null);
 
-		if (logger is null && ElasticOpenTelemetry.BuilderStateTable.TryGetValue(builder, out var state))
-			logger = state.Components.Logger;
+		var callCount = Interlocked.Increment(ref WithElasticDefaultsCallCount);
 
 		if (callCount > 1)
 		{
-			logger?.LogMultipleWithElasticDefaultsCallsWarning(callCount, nameof(LoggerProviderBuilder));
+			logger.LogMultipleWithElasticDefaultsCallsWarning(callCount, nameof(LoggerProviderBuilder));
 		}
 		else
 		{
-			logger?.LogWithElasticDefaultsCallCount(callCount, nameof(LoggerProviderBuilder));
+			logger.LogWithElasticDefaultsCallCount(callCount, nameof(LoggerProviderBuilder));
 		}
 
 		return SignalBuilder.WithElasticDefaults(builder, Signals.Traces, options, components, services, ConfigureBuilder);