Activities for Http Connections, Dns, Sockets and SslStream #103922
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using Xunit; | ||
|
|
||
| namespace System.Net.Test.Common | ||
| { | ||
| internal class ActivityRecorder : IDisposable | ||
| { | ||
| private string _activitySourceName; | ||
| private string _activityName; | ||
|
|
||
| private readonly ActivityListener _listener; | ||
| private List<Activity> _finishedActivities = new(); | ||
|
|
||
| public Predicate<Activity> Filter { get; set; } = _ => true; | ||
| public bool VerifyParent { get; set; } = true; | ||
| public Activity ExpectedParent { get; set; } | ||
|
|
||
| public int Started { get; private set; } | ||
| public int Stopped { get; private set; } | ||
| public Activity LastStartedActivity { get; private set; } | ||
| public Activity LastFinishedActivity { get; private set; } | ||
| public IEnumerable<Activity> FinishedActivities => _finishedActivities; | ||
|
|
||
| public ActivityRecorder(string activitySourceName, string activityName) | ||
| { | ||
| _activitySourceName = activitySourceName; | ||
| _activityName = activityName; | ||
| _listener = new ActivityListener | ||
| { | ||
| ShouldListenTo = (activitySource) => activitySource.Name == _activitySourceName, | ||
| Sample = (ref ActivityCreationOptions<ActivityContext> options) => ActivitySamplingResult.AllData, | ||
| ActivityStarted = (activity) => { | ||
| if (activity.OperationName == _activityName && Filter(activity)) | ||
| { | ||
| if (VerifyParent) | ||
| { | ||
| Assert.Same(ExpectedParent, activity.Parent); | ||
| } | ||
|
|
||
| Started++; | ||
| LastStartedActivity = activity; | ||
| } | ||
| }, | ||
| ActivityStopped = (activity) => { | ||
| if (activity.OperationName == _activityName && Filter(activity)) | ||
| { | ||
| if (VerifyParent) | ||
| { | ||
| Assert.Same(ExpectedParent, activity.Parent); | ||
| } | ||
|
|
||
| Stopped++; | ||
| LastFinishedActivity = activity; | ||
| _finishedActivities.Add(activity); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| ActivitySource.AddActivityListener(_listener); | ||
| } | ||
|
|
||
| public void Dispose() => _listener.Dispose(); | ||
|
|
||
| public void VerifyActivityRecorded(int times) | ||
| { | ||
| Assert.Equal(times, Started); | ||
| Assert.Equal(times, Stopped); | ||
| } | ||
|
|
||
| public Activity VerifyActivityRecordedOnce() | ||
| { | ||
| VerifyActivityRecorded(1); | ||
| return LastFinishedActivity; | ||
| } | ||
| } | ||
|
|
||
| internal static class ActivityAssert | ||
| { | ||
| public static KeyValuePair<string, object> HasTag(Activity activity, string name) | ||
| { | ||
| KeyValuePair<string, object> tag = activity.TagObjects.SingleOrDefault(t => t.Key == name); | ||
| if (tag.Key is null) | ||
| { | ||
| Assert.Fail($"The Activity tags should contain {name}."); | ||
| } | ||
| return tag; | ||
| } | ||
|
|
||
| public static void HasTag<T>(Activity activity, string name, T expectedValue) | ||
| { | ||
| KeyValuePair<string, object> tag = HasTag(activity, name); | ||
| Assert.Equal(expectedValue, (T)tag.Value); | ||
| } | ||
|
|
||
| public static void HasTag<T>(Activity activity, string name, Func<T, bool> verifyValue) | ||
| { | ||
| T? value = (T?)activity.TagObjects.SingleOrDefault(t => t.Key == name).Value; | ||
| Assert.False(value is null, $"The Activity tags should contain {name}."); | ||
| Assert.True(verifyValue(value)); | ||
| } | ||
|
|
||
| public static void HasNoTag(Activity activity, string name) | ||
| { | ||
| bool contains = activity.TagObjects.Any(t => t.Key == name); | ||
| Assert.False(contains, $"The Activity tags should not contain {name}."); | ||
| } | ||
|
|
||
| public static void FinishedInOrder(Activity first, Activity second) | ||
| { | ||
| Assert.True(first.StartTimeUtc + first.Duration < second.StartTimeUtc + second.Duration, $"{first.OperationName} should stop before {second.OperationName}"); | ||
| } | ||
|
|
||
| public static string CamelToSnake(string camel) | ||
| { | ||
| if (string.IsNullOrEmpty(camel)) return camel; | ||
| StringBuilder bld = new(); | ||
| bld.Append(char.ToLower(camel[0])); | ||
| for (int i = 1; i < camel.Length; i++) | ||
| { | ||
| char c = camel[i]; | ||
| if (char.IsUpper(c)) | ||
| { | ||
| bld.Append('_'); | ||
| } | ||
| bld.Append(char.ToLower(c)); | ||
| } | ||
| return bld.ToString(); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.Threading; | ||
|
|
||
| namespace System.Net.Http | ||
| { | ||
| // Implements distributed tracing logic for managing the "HTTP connection_setup" and "HTTP wait_for_connection" Activities. | ||
| internal static class ConnectionSetupDiagnostics | ||
|
There was a problem hiding this comment. Why do we need this extra class? Why can't this be part of There was a problem hiding this comment. In other libs we start/stop the activity together with recording an There was a problem hiding this comment. PS: note that metrics are also handled in separate classes. In HTTP there is a lot of telemetry logic in total, IMO it would be bad to unify it in a single monster class. There was a problem hiding this comment. I still don't think this warrants yet another diagnostics related class in HTTP, but I'm not gonna die on this hill. There was a problem hiding this comment. Agree with the renaming, disagree with code organization preferences 😆 |
||
| { | ||
| private static readonly ActivitySource s_connectionsActivitySource = new ActivitySource(DiagnosticsHandlerLoggingStrings.ConnectionsNamespace); | ||
|
|
||
| public static Activity? StartConnectionSetupActivity(bool isSecure, HttpAuthority authority) | ||
| { | ||
| Activity? activity = null; | ||
| if (s_connectionsActivitySource.HasListeners()) | ||
| { | ||
| // Connection activities should be new roots and not parented under whatever | ||
| // request happens to be in progress when the connection is started. | ||
| Activity.Current = null; | ||
| activity = s_connectionsActivitySource.StartActivity(DiagnosticsHandlerLoggingStrings.ConnectionSetupActivityName); | ||
| } | ||
|
|
||
| if (activity is not null) | ||
| { | ||
| activity.DisplayName = $"HTTP connection_setup {authority.HostValue}:{authority.Port}"; | ||
| if (activity.IsAllDataRequested) | ||
| { | ||
| activity.SetTag("server.address", authority.HostValue); | ||
| activity.SetTag("server.port", authority.Port); | ||
| activity.SetTag("url.scheme", isSecure ? "https" : "http"); | ||
| } | ||
| } | ||
|
|
||
| return activity; | ||
| } | ||
|
|
||
| public static void StopConnectionSetupActivity(Activity activity, Exception? exception, IPEndPoint? remoteEndPoint) | ||
| { | ||
| Debug.Assert(activity is not null); | ||
|
There was a problem hiding this comment. Like in if (connectionSetupActivity is null) return;here. There was a problem hiding this comment. See comment. IMO if we want to get rid of this pattern because we prefer clarity/readability over saving a few cycles, we should do it for all telemetry pillars not only distributed tracing. There was a problem hiding this comment. Actually, I'm being inconsistent since the Start() methods would also need a check if we want to strictly follow the pattern. I will wait for more feedback before changing anything about this. There was a problem hiding this comment.
There was a problem hiding this comment.
My preference would be clarity over microoptimizations everywhere, but some may disagree. |
||
| if (exception is not null) | ||
| { | ||
| ReportError(activity, exception); | ||
| } | ||
| else | ||
| { | ||
| if (activity.IsAllDataRequested && remoteEndPoint is not null) | ||
| { | ||
| activity.SetTag("network.peer.address", remoteEndPoint.Address.ToString()); | ||
| } | ||
| } | ||
|
|
||
| activity.Stop(); | ||
| } | ||
|
|
||
| public static void ReportError(Activity? activity, Exception exception) | ||
| { | ||
| Debug.Assert(exception is not null); | ||
| if (activity is null) return; | ||
| activity.SetStatus(ActivityStatusCode.Error); | ||
|
|
||
| if (activity.IsAllDataRequested) | ||
| { | ||
| DiagnosticsHelper.TryGetErrorType(null, exception, out string? errorType); | ||
| Debug.Assert(errorType is not null, "DiagnosticsHelper.TryGetErrorType() should succeed whenever an exception is provided."); | ||
| activity.SetTag("error.type", errorType); | ||
| } | ||
| } | ||
|
|
||
| public static Activity? StartWaitForConnectionActivity(HttpAuthority authority) | ||
| { | ||
| Activity? activity = s_connectionsActivitySource.StartActivity(DiagnosticsHandlerLoggingStrings.WaitForConnectionActivityName); | ||
ManickaP marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (activity is not null) | ||
| { | ||
| activity.DisplayName = $"HTTP wait_for_connection {authority.HostValue}:{authority.Port}"; | ||
| } | ||
|
|
||
| return activity; | ||
| } | ||
|
|
||
| public static void AddConnectionLinkToRequestActivity(Activity connectionSetupActivity) | ||
| { | ||
| Debug.Assert(connectionSetupActivity is not null); | ||
|
There was a problem hiding this comment. Like in if (connectionSetupActivity is null) return;here. There was a problem hiding this comment. This pattern seems to be generally around for our telemetry everywhere: if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.SomeMethod();I assumed this exists to save a few cycles by avoiding calling a method and deepening the stack on the happy path, so I also applied it for metrics and distributed tracing. |
||
|
|
||
| // We only support links for request activities created by the "System.Net.Http" ActivitySource. | ||
| if (DiagnosticsHandler.s_activitySource.HasListeners()) | ||
| { | ||
| Activity? requestActivity = Activity.Current; | ||
| if (requestActivity?.Source == DiagnosticsHandler.s_activitySource) | ||
| { | ||
| requestActivity.AddLink(new ActivityLink(connectionSetupActivity.Context)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
Experimental?
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.
It turned out that our solution results is an unexpected volume of span links for tools like Azure Monitor, blowing up their UI, see open-telemetry/opentelemetry-specification#4130.
We need to work with OTel and Azure Monitor to introduce a standard way for categorizing links so visualization tools can distinguish them. The plan is to drive this work forward in order to stabilize the feature in .NET 10.