Remove dummy listener

Author: shirhatti

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs

@@ -26,7 +26,6 @@ internal class HostingApplicationDiagnostics
 
         private const string ActivitySourceName = "Microsoft.AspNetCore.Hosting";
         private static readonly ActivitySource _activitySource = new ActivitySource(ActivitySourceName);
-        private ActivityListener? _dummyListener;
 
         private readonly DiagnosticListener _diagnosticListener;
         private readonly ILogger _logger;
@@ -53,19 +52,7 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
             var diagnosticListenerActivityCreationEnabled = (diagnosticListenerEnabled && _diagnosticListener.IsEnabled(ActivityName, httpContext));
             var loggingEnabled = _logger.IsEnabled(LogLevel.Critical);
 
-            // TODO: If loggingEnabled check to change to if ActivityPropogation options are enabled
-            if (loggingEnabled || diagnosticListenerActivityCreationEnabled)
-            {
-                _dummyListener = new ActivityListener()
-                {
-                    ShouldListenTo = activitySource => ReferenceEquals(activitySource, _activitySource),
-                    Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.PropagationData,
-                    SampleUsingParentId = (ref ActivityCreationOptions<string> options) => ActivitySamplingResult.PropagationData
-                };
-                ActivitySource.AddActivityListener(_dummyListener);
-            }
-
-            context.Activity = StartActivity(httpContext, diagnosticListenerActivityCreationEnabled, out var hasDiagnosticListener);
+            context.Activity = StartActivity(httpContext, loggingEnabled, diagnosticListenerActivityCreationEnabled, out var hasDiagnosticListener);
             context.HasDiagnosticListener = hasDiagnosticListener;
 
             if (diagnosticListenerEnabled)
@@ -77,10 +64,6 @@ public void BeginRequest(HttpContext httpContext, HostingApplication.Context con
                 }
             }
 
-            // Dispose dummy listener
-            // TODO: How do we avoid allocating a new listener on each request
-            _dummyListener?.Dispose();
-
             // To avoid allocation, return a null scope if the logger is not on at least to some degree.
             if (loggingEnabled)
             {
@@ -263,19 +246,18 @@ private static void RecordRequestStartEventLog(HttpContext httpContext)
         }
 
         [MethodImpl(MethodImplOptions.NoInlining)]
-        private Activity? StartActivity(HttpContext httpContext, bool diagnosticListenerActivityCreationEnabled, out bool hasDiagnosticListener)
+        private Activity? StartActivity(HttpContext httpContext, bool loggingEnabled, bool diagnosticListenerActivityCreationEnabled, out bool hasActivityStartDiagnosticListener)
         {
-            hasDiagnosticListener = false;
+            hasActivityStartDiagnosticListener = false;
             if (diagnosticListenerActivityCreationEnabled)
             {
                 if (_diagnosticListener.IsEnabled(ActivityStartKey))
                 {
-                    hasDiagnosticListener = true;
+                    hasActivityStartDiagnosticListener = true;
                 }
             }
-
             // Short-circuit to avoid doing an expensive header lookup
-            if (!(hasDiagnosticListener || _activitySource.HasListeners()))
+            if (!(diagnosticListenerActivityCreationEnabled || _activitySource.HasListeners() || loggingEnabled))
             {
                 return null;
             }
@@ -285,48 +267,57 @@ private static void RecordRequestStartEventLog(HttpContext httpContext)
             {
                 headers.TryGetValue(HeaderNames.RequestId, out requestId);
             }
+
             var activity = _activitySource.StartActivity(ActivityName, ActivityKind.Server, requestId);
-            if (activity is not null)
+            bool ActivityStarted = true;
+            if (activity is null)
             {
-                if (!StringValues.IsNullOrEmpty(requestId))
-                {
-                    // activity.SetParentId(requestId);
-                    if (headers.TryGetValue(HeaderNames.TraceState, out var traceState))
-                    {
-                        activity.TraceStateString = traceState;
-                    }
-
-                    // We expect baggage to be empty by default
-                    // Only very advanced users will be using it in near future, we encourage them to keep baggage small (few items)
-                    var baggage = headers.GetCommaSeparatedValues(HeaderNames.Baggage);
-                    if (baggage.Length == 0)
-                    {
-                        baggage = headers.GetCommaSeparatedValues(HeaderNames.CorrelationContext);
-                    }
+                ActivityStarted = false;
+                activity = new Activity(ActivityName);
+            }
 
-                    // AddBaggage adds items at the beginning  of the list, so we need to add them in reverse to keep the same order as the client
-                    // An order could be important if baggage has two items with the same key (that is allowed by the contract)
-                    for (var i = baggage.Length - 1; i >= 0; i--)
-                    {
-                        if (NameValueHeaderValue.TryParse(baggage[i], out var baggageItem))
-                        {
-                            activity.AddBaggage(baggageItem.Name.ToString(), HttpUtility.UrlDecode(baggageItem.Value.ToString()));
-                        }
-                    }
+            if (!StringValues.IsNullOrEmpty(requestId))
+            {
+                activity.SetParentId(requestId);
+                if (headers.TryGetValue(HeaderNames.TraceState, out var traceState))
+                {
+                    activity.TraceStateString = traceState;
                 }
 
-                if (diagnosticListenerActivityCreationEnabled)
+                // We expect baggage to be empty by default
+                // Only very advanced users will be using it in near future, we encourage them to keep baggage small (few items)
+                var baggage = headers.GetCommaSeparatedValues(HeaderNames.Baggage);
+                if (baggage.Length == 0)
                 {
-                    // Review: Breaking change: We will no longer fire OnActivityImport before Activity.Start()
-                    _diagnosticListener.OnActivityImport(activity, httpContext);
+                    baggage = headers.GetCommaSeparatedValues(HeaderNames.CorrelationContext);
+                }
 
-                    if (hasDiagnosticListener)
+                // AddBaggage adds items at the beginning  of the list, so we need to add them in reverse to keep the same order as the client
+                // An order could be important if baggage has two items with the same key (that is allowed by the contract)
+                for (var i = baggage.Length - 1; i >= 0; i--)
+                {
+                    if (NameValueHeaderValue.TryParse(baggage[i], out var baggageItem))
                     {
-                        //Review: Do we need to explicity call this?
-                        _diagnosticListener.Write(ActivityStartKey, httpContext);
+                        activity.AddBaggage(baggageItem.Name.ToString(), HttpUtility.UrlDecode(baggageItem.Value.ToString()));
                     }
                 }
             }
+
+            if (diagnosticListenerActivityCreationEnabled)
+            {
+                _diagnosticListener.OnActivityImport(activity, httpContext);
+            }
+
+            if (!ActivityStarted)
+            {
+                activity.Start();
+            }
+            
+            if (hasActivityStartDiagnosticListener)
+            {
+                _diagnosticListener.Write(ActivityStartKey, httpContext);
+            }
+
             return activity;
         }