diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md index d792fcc5680..89b00879f77 100644 --- a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md +++ b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +* Instrumentation for `HttpWebRequest` no longer store raw objects like + `HttpWebRequest` in Activity.CustomProperty. To enrich activity, use the + Enrich action on the instrumentation. + ([#1261](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1407)) + ## 0.7.0-beta.1 Released 2020-Oct-16 diff --git a/src/OpenTelemetry.Instrumentation.Http/HttpWebRequestInstrumentationOptions.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/HttpWebRequestInstrumentationOptions.netfx.cs index d84adf2d0d6..bde54ec8ca6 100644 --- a/src/OpenTelemetry.Instrumentation.Http/HttpWebRequestInstrumentationOptions.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/HttpWebRequestInstrumentationOptions.netfx.cs @@ -16,6 +16,7 @@ #if NETFRAMEWORK using System; +using System.Diagnostics; using System.Net; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Instrumentation.Http.Implementation; @@ -50,6 +51,17 @@ public class HttpWebRequestInstrumentationOptions /// public Func Filter { get; set; } + /// + /// Gets or sets an action to enrich an Activity. + /// + /// + /// : the activity being enriched. + /// string: the name of the event. + /// object: the raw object from which additional information can be extracted to enrich the activity. + /// The type of this object depends on the event, which is given by the above parameter. + /// + public Action Enrich { get; set; } + internal bool EventFilter(HttpWebRequest request) { try diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs index 29eab31b312..139747c1028 100644 --- a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs +++ b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs @@ -22,8 +22,6 @@ using System.Reflection; using System.Reflection.Emit; using System.Runtime.CompilerServices; -using System.Text; -using OpenTelemetry.Context; using OpenTelemetry.Context.Propagation; using OpenTelemetry.Trace; @@ -41,10 +39,6 @@ internal static class HttpWebRequestActivitySource public const string ActivitySourceName = "OpenTelemetry.HttpWebRequest"; public const string ActivityName = ActivitySourceName + ".HttpRequestOut"; - public const string RequestCustomPropertyName = "OTel.HttpWebRequest.Request"; - public const string ResponseCustomPropertyName = "OTel.HttpWebRequest.Response"; - public const string ExceptionCustomPropertyName = "OTel.HttpWebRequest.Exception"; - internal static readonly Func> HttpWebRequestHeaderValuesGetter = (request, name) => request.Headers.GetValues(name); internal static readonly Action HttpWebRequestHeaderValuesSetter = (request, name, value) => request.Headers.Add(name, value); @@ -104,7 +98,15 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A if (activity.IsAllDataRequested) { - activity.SetCustomProperty(RequestCustomPropertyName, request); + try + { + Options.Enrich?.Invoke(activity, "OnStartActivity", request); + } + catch (Exception ex) + { + HttpInstrumentationEventSource.Log.EnrichmentException(ex); + } + activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method); activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri)); activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString); @@ -120,7 +122,15 @@ private static void AddResponseTags(HttpWebResponse response, Activity activity) { if (activity.IsAllDataRequested) { - activity.SetCustomProperty(ResponseCustomPropertyName, response); + try + { + Options.Enrich?.Invoke(activity, "OnStopActivity", response); + } + catch (Exception ex) + { + HttpInstrumentationEventSource.Log.EnrichmentException(ex); + } + activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)response.StatusCode); activity.SetStatus( @@ -138,7 +148,14 @@ private static void AddExceptionTags(Exception exception, Activity activity) return; } - activity.SetCustomProperty(ExceptionCustomPropertyName, exception); + try + { + Options.Enrich?.Invoke(activity, "OnException", exception); + } + catch (Exception ex) + { + HttpInstrumentationEventSource.Log.EnrichmentException(ex); + } Status status; if (exception is WebException wexc) diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs index 123f2373f5e..da6d4143a68 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestActivitySourceTests.netfx.cs @@ -24,7 +24,6 @@ using System.Net.Http; using System.Threading; using System.Threading.Tasks; -using OpenTelemetry.Context; using OpenTelemetry.Instrumentation.Http.Implementation; using OpenTelemetry.Tests; using OpenTelemetry.Trace; @@ -34,6 +33,7 @@ namespace OpenTelemetry.Instrumentation.Http.Tests { public class HttpWebRequestActivitySourceTests : IDisposable { + private static bool validateBaggage; private readonly IDisposable testServer; private readonly string testServerHost; private readonly int testServerPort; @@ -41,6 +41,13 @@ public class HttpWebRequestActivitySourceTests : IDisposable static HttpWebRequestActivitySourceTests() { + HttpWebRequestInstrumentationOptions options = new HttpWebRequestInstrumentationOptions + { + Enrich = ActivityEnrichment, + }; + + HttpWebRequestActivitySource.Options = options; + // Need to touch something in HttpWebRequestActivitySource to do the static injection. GC.KeepAlive(HttpWebRequestActivitySource.Options); } @@ -179,15 +186,12 @@ public async Task TestBasicReceiveAndResponseEvents(string method, string queryS Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); // Check to make sure: The first record must be a request, the next record must be a response. - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); + Activity activity = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); VerifyActivityStartTags(this.hostNameAndPort, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out var stopEvent)); Assert.Equal("Stop", stopEvent.Key); - HttpWebResponse response = (HttpWebResponse)stopEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ResponseCustomPropertyName); - Assert.NotNull(response); VerifyActivityStopTags(200, "OK", activity); } @@ -362,15 +366,12 @@ public async Task TestBasicReceiveAndResponseWebRequestEvents(string method, int Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); // Check to make sure: The first record must be a request, the next record must be a response. - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); + Activity activity = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); VerifyActivityStartTags(this.hostNameAndPort, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out var stopEvent)); Assert.Equal("Stop", stopEvent.Key); - HttpWebResponse response = (HttpWebResponse)stopEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ResponseCustomPropertyName); - Assert.NotNull(response); VerifyActivityStopTags(200, "OK", activity); } @@ -400,16 +401,7 @@ public async Task TestTraceStateAndBaggage() Assert.Equal(2, eventRecords.Records.Count()); // Check to make sure: The first record must be a request, the next record must be a response. - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); - - var traceparent = startRequest.Headers["traceparent"]; - var tracestate = startRequest.Headers["tracestate"]; - var baggage = startRequest.Headers["baggage"]; - Assert.NotNull(traceparent); - Assert.Equal("some=state", tracestate); - Assert.Equal("k=v", baggage); - Assert.StartsWith($"00-{parent.TraceId.ToHexString()}-", traceparent); - Assert.Matches("^[0-9a-f]{2}-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$", traceparent); + _ = AssertFirstEventWasStart(eventRecords); } finally { @@ -480,17 +472,12 @@ public async Task TestResponseWithoutContentEvents(string method) Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); // Check to make sure: The first record must be a request, the next record must be a response. - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); + Activity activity = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); VerifyActivityStartTags(this.hostNameAndPort, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out var stopEvent)); Assert.Equal("Stop", stopEvent.Key); - HttpWebRequest stopRequest = (HttpWebRequest)stopEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - Assert.Equal(startRequest, stopRequest); - HttpWebResponse stopResponse = (HttpWebResponse)stopEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ResponseCustomPropertyName); - Assert.NotNull(stopResponse); VerifyActivityStopTags(204, "No Content", activity); } @@ -550,16 +537,11 @@ public async Task TestRequestWithException(string method) Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); // Check to make sure: The first record must be a request, the next record must be an exception. - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); + Activity activity = AssertFirstEventWasStart(eventRecords); VerifyActivityStartTags(null, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair exceptionEvent)); Assert.Equal("Stop", exceptionEvent.Key); - HttpWebRequest exceptionRequest = (HttpWebRequest)exceptionEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - Assert.Equal(startRequest, exceptionRequest); - Exception exceptionException = (Exception)exceptionEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ExceptionCustomPropertyName); - Assert.Equal(webException, exceptionException); Assert.NotNull(activity.GetTagValue(SpanAttributeConstants.StatusCodeKey)); Assert.NotNull(activity.GetTagValue(SpanAttributeConstants.StatusDescriptionKey)); @@ -594,13 +576,11 @@ public async Task TestCanceledRequest(string method) Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); + Activity activity = AssertFirstEventWasStart(eventRecords); VerifyActivityStartTags(this.hostNameAndPort, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair exceptionEvent)); Assert.Equal("Stop", exceptionEvent.Key); - Exception exceptionException = (Exception)exceptionEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ExceptionCustomPropertyName); Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey)); Assert.Null(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey)); @@ -635,13 +615,11 @@ public async Task TestSecureTransportFailureRequest(string method) Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); + Activity activity = AssertFirstEventWasStart(eventRecords); VerifyActivityStartTags(null, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair exceptionEvent)); Assert.Equal("Stop", exceptionEvent.Key); - Exception exceptionException = (Exception)exceptionEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ExceptionCustomPropertyName); Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey)); Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey)); @@ -679,13 +657,11 @@ public async Task TestSecureTransportRetryFailureRequest(string method) Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - (Activity activity, HttpWebRequest startRequest) = AssertFirstEventWasStart(eventRecords); - VerifyHeaders(startRequest); + Activity activity = AssertFirstEventWasStart(eventRecords); VerifyActivityStartTags(this.hostNameAndPort, method, url, activity); Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair exceptionEvent)); Assert.Equal("Stop", exceptionEvent.Key); - Exception exceptionException = (Exception)exceptionEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.ExceptionCustomPropertyName); Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusCodeKey)); Assert.NotNull(exceptionEvent.Value.GetTagValue(SpanAttributeConstants.StatusDescriptionKey)); @@ -694,6 +670,7 @@ public async Task TestSecureTransportRetryFailureRequest(string method) [Fact] public async Task TestInvalidBaggage() { + validateBaggage = true; Baggage .SetBaggage("key", "value") .SetBaggage("bad/key", "value") @@ -710,13 +687,7 @@ public async Task TestInvalidBaggage() Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Start")); Assert.Equal(1, eventRecords.Records.Count(rec => rec.Key == "Stop")); - WebRequest thisRequest = (WebRequest)eventRecords.Records.First().Value.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - string[] baggage = thisRequest.Headers["Baggage"].Split(','); - - Assert.Equal(3, baggage.Length); - Assert.Contains("key=value", baggage); - Assert.Contains("bad%2Fkey=value", baggage); - Assert.Contains("goodkey=bad%2Fvalue", baggage); + validateBaggage = false; } /// @@ -774,80 +745,27 @@ public void TestMultipleConcurrentRequests() pair.Key == "Start" || pair.Key == "Stop", "An unexpected event of name " + pair.Key + "was received"); - - WebRequest request = (WebRequest)activity.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - Assert.Equal("HttpWebRequest", request.GetType().Name); - - if (pair.Key == "Start") - { - // Make sure this is an URL that we recognize. If not, just skip - if (!requestData.TryGetValue(request.RequestUri, out var tuple)) - { - continue; - } - - // all requests have traceparent with proper parent Id - var traceparent = request.Headers["traceparent"]; - Assert.StartsWith($"00-{parentActivity.TraceId.ToHexString()}-", traceparent); - - Assert.Null(requestData[request.RequestUri]); - requestData[request.RequestUri] = - new Tuple(request, null); - } - else - { - // This must be the response. - WebResponse response = (WebResponse)activity.GetCustomProperty(HttpWebRequestActivitySource.ResponseCustomPropertyName); - Assert.Equal("HttpWebResponse", response.GetType().Name); - - // By the time we see the response, the request object may already have been redirected with a different - // url. Hence, it's not reliable to just look up requestData by the URL/hostname. Instead, we have to look - // through each one and match by object reference on the request object. - Tuple tuple = null; - foreach (Tuple currentTuple in requestData.Values) - { - if (currentTuple != null && currentTuple.Item1 == request) - { - // Found it! - tuple = currentTuple; - break; - } - } - - // Update the tuple with the response object - Assert.NotNull(tuple); - requestData[request.RequestUri] = - new Tuple(request, response); - } - } - - // Finally, make sure we have request and response objects for every successful request - foreach (KeyValuePair> pair in requestData) - { - if (successfulTasks.Any(t => t.Key == pair.Key)) - { - Assert.NotNull(pair.Value); - Assert.NotNull(pair.Value.Item1); - Assert.NotNull(pair.Value.Item2); - } } } - private static (Activity, HttpWebRequest) AssertFirstEventWasStart(ActivitySourceRecorder eventRecords) + private static Activity AssertFirstEventWasStart(ActivitySourceRecorder eventRecords) { Assert.True(eventRecords.Records.TryDequeue(out KeyValuePair startEvent)); Assert.Equal("Start", startEvent.Key); - HttpWebRequest startRequest = (HttpWebRequest)startEvent.Value.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - Assert.NotNull(startRequest); - return (startEvent.Value, startRequest); + return startEvent.Value; } private static void VerifyHeaders(HttpWebRequest startRequest) { + var tracestate = startRequest.Headers["tracestate"]; + Assert.Equal("some=state", tracestate); + + var baggage = startRequest.Headers["baggage"]; + Assert.Equal("k=v", baggage); + var traceparent = startRequest.Headers["traceparent"]; Assert.NotNull(traceparent); - Assert.Matches("^[0-9a-f][0-9a-f]-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f][0-9a-f]$", traceparent); - Assert.Null(startRequest.Headers["tracestate"]); + Assert.Matches("^[0-9a-f]{2}-[0-9a-f]{32}-[0-9a-f]{16}-[0-9a-f]{2}$", traceparent); } private static void VerifyActivityStartTags(string hostNameAndPort, string method, string url, Activity activity) @@ -868,6 +786,44 @@ private static void VerifyActivityStopTags(int statusCode, string statusText, Ac Assert.Equal(statusText, activity.GetTagValue(SpanAttributeConstants.StatusDescriptionKey)); } + private static void ActivityEnrichment(Activity activity, string method, object obj) + { + switch (method) + { + case "OnStartActivity": + Assert.True(obj is HttpWebRequest); + VerifyHeaders(obj as HttpWebRequest); + + if (validateBaggage) + { + ValidateBaggage(obj as HttpWebRequest); + } + + break; + + case "OnStopActivity": + Assert.True(obj is HttpWebResponse); + break; + + case "OnException": + Assert.True(obj is Exception); + break; + + default: + break; + } + } + + private static void ValidateBaggage(HttpWebRequest request) + { + string[] baggage = request.Headers["Baggage"].Split(','); + + Assert.Equal(3, baggage.Length); + Assert.Contains("key=value", baggage); + Assert.Contains("bad%2Fkey=value", baggage); + Assert.Contains("goodkey=bad%2Fvalue", baggage); + } + private string BuildRequestUrl(bool useHttps = false, string path = "echo", string queryString = null) { return $"{(useHttps ? "https" : "http")}://{this.testServerHost}:{this.testServerPort}/{path}{(string.IsNullOrWhiteSpace(queryString) ? string.Empty : $"?{queryString}")}"; diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.netfx.cs b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.netfx.cs index b161282f7f7..75dd01b7a3e 100644 --- a/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.netfx.cs +++ b/test/OpenTelemetry.Instrumentation.Http.Tests/HttpWebRequestTests.netfx.cs @@ -35,7 +35,7 @@ public partial class HttpWebRequestTests [Theory] [MemberData(nameof(TestData))] - public void HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCase tc) + public void HttpOutCallsAreCollectedSuccessfully(HttpTestData.HttpOutTestCase tc) { using var serverLifeTime = TestHttpServer.RunServer( (ctx) => @@ -51,7 +51,11 @@ public void HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCa using var shutdownSignal = Sdk.CreateTracerProviderBuilder() .SetResource(expectedResource) .AddProcessor(activityProcessor.Object) - .AddHttpWebRequestInstrumentation(options => options.SetHttpFlavor = tc.SetHttpFlavor) + .AddHttpWebRequestInstrumentation(options => + { + options.SetHttpFlavor = tc.SetHttpFlavor; + options.Enrich = ActivityEnrichment; + }) .Build(); tc.Url = HttpTestData.NormalizeValues(tc.Url, host, port); @@ -136,7 +140,7 @@ public void HttpOutCallsAreCollectedSuccessfullyAsync(HttpTestData.HttpOutTestCa } [Fact] - public void DebugIndividualTestAsync() + public void DebugIndividualTest() { var serializer = new JsonSerializer(); var input = serializer.Deserialize(new JsonTextReader(new StringReader(@" @@ -158,22 +162,33 @@ public void DebugIndividualTestAsync() } } "))); - this.HttpOutCallsAreCollectedSuccessfullyAsync(input); + this.HttpOutCallsAreCollectedSuccessfully(input); } private static void ValidateHttpWebRequestActivity(Activity activityToValidate, Resources.Resource expectedResource, bool responseExpected) { Assert.Equal(ActivityKind.Client, activityToValidate.Kind); Assert.Equal(expectedResource, activityToValidate.GetResource()); - var request = activityToValidate.GetCustomProperty(HttpWebRequestActivitySource.RequestCustomPropertyName); - Assert.NotNull(request); - Assert.True(request is HttpWebRequest); + } - if (responseExpected) + private static void ActivityEnrichment(Activity activity, string method, object obj) + { + switch (method) { - var response = activityToValidate.GetCustomProperty(HttpWebRequestActivitySource.ResponseCustomPropertyName); - Assert.NotNull(response); - Assert.True(response is HttpWebResponse); + case "OnStartActivity": + Assert.True(obj is HttpWebRequest); + break; + + case "OnStopActivity": + Assert.True(obj is HttpWebResponse); + break; + + case "OnException": + Assert.True(obj is Exception); + break; + + default: + break; } } }