HttpWebRequest ActivitySource #694

CodeBlanch · 2020-05-22T05:53:19Z

Resolves #680.
Resolves #639.

Changes

Updated HttpWebRequestDiagnosticSource to use the new .NET 5 ActivitySource API.

Work remaining:

Update unit tests.
Update sample.

/cc @cijothomas @tarekgh

Checklist

I ran Unit Tests locally.

…Source API.

CodeBlanch · 2020-05-22T06:04:13Z

@tarekgh @cijothomas Request & response conversion was pretty straight-forward. Previously we had an option for setting HTTP flavor or not, wasn't sure how to bring that forward. The exception case got messy real fast. Before we had a mapping of the error to OT Status, I couldn't bring that forward. I know we're working on that. But there are also cases where we don't get a status code, just an exception. I was not sure what to do in that case. I feel like there should be a way to mark the Activity as faulted and set the exception? I'll drop a couple of comments on the code areas to highlight the before and after.

PS: I have it calling SetCustomProperty to put the raw objects on the Activity at the moment. Not sure if we want that? OT isn't using it, that would be for downstream users?

CodeBlanch · 2020-05-22T06:06:34Z

...metry.Instrumentation.Dependencies/Implementation/HttpWebRequestDiagnosticListener.net461.cs

-        }
-
-        private static void ProcessException(TelemetrySpan span, Exception exception)
-        {


Previous exception handling is here. Three cases: 1) We have a response code (brought forward), 2) We have a WebExceptionStatus (gap), and 3) Unknown exception (kind of brought forward, I set the exception.Message as a tag).

CodeBlanch · 2020-05-22T06:07:28Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

            }
+
+            if (exception is WebException wexc && wexc.Response is HttpWebResponse response)


Here is the new exception handling.

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

cijothomas · 2020-05-22T13:45:17Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+            catch (Exception ex)
+            {
+                // If anything went wrong, just no-op. Write an event so at least we can find out.
+                var activity = ActivitySource.StartActivity(InitializationFailedActivityName, ActivityKind.Server);


This could be an abuse of activity - activity should represent an operation.. something with a begin and end notion.
Could we just log to EventSource at Error level instead?

I updated to use InstrumentationEventSource. Please review and LMK if that works!

cijothomas · 2020-05-22T13:49:31Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+            var activity = ActivitySource.StartActivity(
+                ActivityName,
+                ActivityKind.Client,
+                Activity.Current?.Context ?? default,


We shouldn't need to pass Activity.Current.context here.. It should be implicit.
We should pass context, only if we extracted context from remote.

I believe you are passing it because the StartActivity method expects context as well, when tags are to be passed at creation time?

I could be wrong here, so asking @tarekgh for expert opinion :)

Yes I ran into a bit of a "chicken-and-egg" situation here. I wanted tags to be present when Activity is started so anyone listening would have some data to make more intelligent sampling decisions. But ActivitySource API only gives me .StartActivity operations. No way to create Activity, build it up, and then Start it. So I tried to pass in everything I could on the StartActivity call which required then the context.

@tarekgh Any guidance to offer? What should I pass for context here?

I believe you can pass just default here. That will cause the Activity created and the parent context will automatically picked from Activity.Current.

In reply to: 429364884 [](ancestors = 429364884)

cijothomas · 2020-05-22T13:57:02Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

-                    this.Write(activity.OperationName + ".Start", new { Request = request });
-                }
-            }
+            activity.SetCustomProperty("request", request);


Did our reflection hack DiagnosticSource inject w3ctraceparent headers? Or was it done at Listener?
In the new model, listener won't be able to do it, as "request" is part of customProperty, and not available at StartActivity callback.

Header injection is done right before the SetCustomProperty call as part of the InstrumentRequest call. Line 481 on the diff. No change in that regard.

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

cijothomas · 2020-05-22T14:02:54Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+            activity.AddTag(SpanAttributeConstants.HttpStatusCodeKey, ((int)response.StatusCode).ToString());
+            activity.AddTag(SpanAttributeConstants.HttpStatusTextKey, response.StatusDescription);
+
+            activity.SetCustomProperty("response", response);


I am looking for where activity.Stop is called. Can you point me to where we stop activity?

It's called on line 205 in ProcessResult right after RaiseResponseEvent or RaiseExceptionEvent is fired.

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

Line 205 in 8e92f13

activity.Stop();

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

cijothomas · 2020-05-22T14:05:30Z

src/OpenTelemetry.Instrumentation.Dependencies/TracerBuilderExtensions.cs

                .AddInstrumentation((t) => new SqlClientInstrumentation(t, sqlOptions));
        }
+
+        /// <summary>
+        /// Enables the outgoing requests automatic data collection.


lets make the comment more explicitly state that this enables outgoing calls made by HttpWebRequest, and is applicable in .NET Framework only.. something of that tone.
Maybe rename the method itself to be more specific about httpwebrequest.

We can later add a common AddAllDependencyInstrumentation helper covering all dependencies - thoughts?

My thinking here was this one call would spin up all our dependency instrumentation, similar to what public static TracerBuilder AddDependencyInstrumentation(this TracerBuilder builder) does today. Right now it just has the one .NET Framework thing because we haven't converted the others, but that will be changing right?

It should look something like this at the end of the day:

public static OpenTelemetryBuilder AddDependencyInstrumentation( this OpenTelemetryBuilder builder, Action<HttpClientInstrumentationOptions> configureHttpInstrumentationOptions = null, Action<SqlClientInstrumentationOptions> configureSqlInstrumentationOptions = null) { if (builder == null) { throw new ArgumentNullException(nameof(builder)); } builder.AddActivitySource(AzureClientsActivitySource.ActivitySourceName); builder.AddActivitySource(AzurePipelineActivitySource.ActivitySourceName); builder.AddActivitySource<HttpClientInstrumentationOptions>(HttpClientActivitySource.ActivitySourceName, configureHttpInstrumentationOptions); builder.AddActivitySource<SqlClientInstrumentationOptions>(SqlClientActivitySource.ActivitySourceName, configureSqlInstrumentationOptions); #if NET461 GC.KeepAlive(HttpWebRequestActivitySource.Instance); builder.AddActivitySource(HttpWebRequestActivitySource.ActivitySourceName); #endif return builder; }

So it's not specific to .NET Framework. I also invented a way to pass options, looks like we have a gap there.

If we had this, then we would remove the existing TraceBuilder extensions?

I actually to say we need to have separate methods for each dependency - like AddSqlDependencyInstrumentation, AddHttpClientInstrumentation etc. So users can pick individual ones if they want.

We'll also have a method like AddDependencyInstrumentation() which most users would use. (exactly the one you shared above).

(having separate method allows us to turn on/off individual collections. very helpful when investigating some leak or issues, and want to isolate to a particular instrumentation)

And you are right about removing TracerBuilder extension method- Once we finish span->activity work, there would be no TracerBuilder!

I split the new stuff out into OpenTelemetryBuilderExtensions.cs and added a method for registering all sources (which will grow) or the HttpWebRequest source only. LMK.

cijothomas

Excellent start @CodeBlanch . I have added comments. I am not an expert in the httwebrequest reflection hack, so some comments may not make sense! let me know.

tarekgh · 2020-05-22T19:29:20Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+
+            InstrumentRequest(request, activity);
+
+            activity.SetCustomProperty("request", request);


request [](start = 40, length = 7)

I would recommend to name the property name in more specific way to avoid collision for anyone else using the Activity with other scenarios.

maybe you can name something like HttpWebRequest, or HttpWebRequest (I am using underscores here but you can use any other symbol),. just to be unlikely wanyone else will use that name.

This should apply to the response and exceptions too.

I updated so the custom properties are named "HttpWebRequest.Request", "HttpWebRequest.Response", and "HttpWebRequest.Exception." The idea being they now have a kind of namespace so they don't collide with what the user might want to do.

tarekgh · 2020-05-22T19:39:43Z

The result of the latest discussions regarding OT Status can be seen here open-telemetry/opentelemetry-specification#306 (comment)

For your case, I would recommend setting some tag indicating if there is error or not (with setting proper error code if needed). My understanding http headers also have field to set the error code if needed. For exception, either you want o include the whole exception object using SetCustomProperty or you may call exception.ToString() and then stick the result as a tag instead. I would recommend using tags if you don't really need the exception object.

In reply to: 632503031 [](ancestors = 632503031)

CodeBlanch · 2020-05-23T19:07:02Z

samples/Exporters/AspNet/Global.asax.cs

+
+            this.openTelemetry = OpenTelemetrySdk.EnableOpenTelemetry(
+                (builder) => builder.AddDependencyInstrumentation()
+                .UseJaegerActivityExporter(c =>


@pjanotti FYI I updated to use JaegerActivityExporter since it was merged while I was working on this.

CodeBlanch · 2020-05-23T19:08:19Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

        {
+            activity.DisplayName = BuildDisplayName(request);


@cijothomas @tarekgh We want to set Activity.DisplayName to what was previously Span.Name right?

that is right.

CodeBlanch · 2020-05-23T19:11:14Z

src/OpenTelemetry/Trace/Configuration/OpenTelemetrySdk.cs

        /// <remarks>
        /// Basic implementation only. Most logic from TracerBuilder will be ported here.
        /// </remarks>
-        public static void EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpenTelemetryBuilder)
+        public static IDisposable EnableOpenTelemetry(Action<OpenTelemetryBuilder> configureOpenTelemetryBuilder)


@cijothomas I couldn't make the unit tests work without a way to destroy the ActivityListener we create internally so I return it as an IDisposable here. But we'll also need to also stop any ActivityProcessors/ActivityExporters on application shutdown right?

Yes, we should gracefully call processors & exporters to ensure they flush active spans before the application closes. I don't see a problem with making this IDisposable but we'll need to ensure docs and examples are updated to include this as a best practice too.

test/OpenTelemetry.Instrumentation.Dependencies.Tests/HttpWebRequestTests.Basic.net461.cs

CodeBlanch · 2020-05-23T19:15:10Z

test/OpenTelemetry.Instrumentation.Dependencies.Tests/HttpWebRequestTests.net461.cs


+            /* TBD: Span Status is not currently support on Activity.


Known gaps with Span.Status & Activity/Source.

…ncies project.

cijothomas · 2020-05-27T19:47:29Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+            }
+            else
+            {
+                activity.AddTag("error", exception.Message);


this is not a wellknown tagname right? Were we doing this previously? Most exporters export selected, well-known tags, so this may not be used

This is due to the Status gap with Span. Previous logic is here:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestDiagnosticListener.net461.cs

Lines 123 to 161 in acaec5d

private static void ProcessException(TelemetrySpan span, Exception exception)

{

if (exception is WebException wexc)

{

if (wexc.Response is HttpWebResponse response)

{

span.PutHttpStatusCode((int)response.StatusCode, response.StatusDescription);

return;

}

switch (wexc.Status)

{

case WebExceptionStatus.Timeout:

span.Status = Status.DeadlineExceeded;

return;

case WebExceptionStatus.NameResolutionFailure:

span.Status = Status.InvalidArgument.WithDescription(exception.Message);

return;

case WebExceptionStatus.SendFailure:

case WebExceptionStatus.ConnectFailure:

case WebExceptionStatus.SecureChannelFailure:

case WebExceptionStatus.TrustFailure:

span.Status = Status.FailedPrecondition.WithDescription(exception.Message);

return;

case WebExceptionStatus.ServerProtocolViolation:

span.Status = Status.Unimplemented.WithDescription(exception.Message);

return;

case WebExceptionStatus.RequestCanceled:

span.Status = Status.Cancelled;

return;

case WebExceptionStatus.MessageLengthLimitExceeded:

span.Status = Status.ResourceExhausted.WithDescription(exception.Message);

return;

}

}

span.Status = Status.Unknown.WithDescription(exception.Message);

}

}

Today we get exception.Message as Status.Description ("ot.status_description") in cases where the exception wasn't tied to an Http status code. Agree "error" feels weird but not sure what to do. I could switch to "ot.status_description" but that also feels weird without "ot.status_code" also being sent? This was more or less a temporary thing while we wait to find out what we'll be doing about the Status gap.

Agreed: this is temporary while OTel defines status and error reporting. That said error is a semantic tag in OpenTracing - https://docs.google.com/spreadsheets/d/1c8QDEBrN6ihVlTiz7Ak_CE-o2flR7XgdT8yFQeolPRc/edit#gid=2124992250 so it seems better to avoid it.

For the time being, setting both ot.status_description and ot.status_code seem reasonable to me.

Agree about setting ot.status_description and ot.status_code

cijothomas · 2020-05-27T19:48:21Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+                activity.AddTag("error", exception.Message);
+            }
+
+            activity.SetCustomProperty("HttpWebRequest.Exception", exception);


if some processor/exporter wants to add exception.Message, then can do so by reading this exception. That means we can avoid adding the "error" tag.

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

CodeBlanch · 2020-05-28T19:03:44Z

@cijothomas Updated to check Activity.IsAllDataRequested based on our discussion.

MikeGoldsmith

I think this is a good start and accept then iterate as needed.

pjanotti

I agree w/ @MikeGoldsmith: LGTM, I have a couple of Qs/suggestions but it is better to address them separately.

pjanotti · 2020-05-29T22:28:15Z

src/OpenTelemetry.Api/Trace/SpanHelper.cs

+    /// </summary>
+    public static class SpanHelper
+    {
+        private static readonly ConcurrentDictionary<StatusCanonicalCode, string> CanonicalCodeToStringCache = new ConcurrentDictionary<StatusCanonicalCode, string>();


Suggestion for a separate PR depending if Status doesn't change much: since there are only 17 canonical codes an array should suffice.

Only 17 he says :) I updated the PR since I was in there trying to work on some of your other feedback.

pjanotti · 2020-05-29T22:28:41Z

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/HttpHandlerDiagnosticListener.cs

@@ -73,24 +73,24 @@ public override void OnStartActivity(Activity activity, object payload)
                return;
            }

-            this.Tracer.StartActiveSpanFromActivity(Constants.HttpSpanPrefix + request.Method, activity, SpanKind.Client, out var span);
+            this.Tracer.StartActiveSpanFromActivity(HttpTagHelper.GetOperationNameForHttpMethod(request.Method), activity, SpanKind.Client, out var span);


Suggestion: there are reasons to not include the path by default in the name but it is very useful on the root span, it will be good to have an option to add it to the span name.

pjanotti · 2020-05-29T22:29:00Z

src/OpenTelemetry.Instrumentation.Dependencies/Implementation/HttpTagHelper.cs

+            return hostTagValue;
+        }
+
+        private static string ConvertMethodToOperationName(string method) => $"HTTP {method}";


I interpret the addition of "HTTP " as a suggestion of the spec, in this case, it seems reasonable to directly use the Method field and avoid all caching and related. Or at least have an option for it. We can have a better conversation about it in a separate PR.

That was a more recent change. Check out: #633. I agree having options to control it would be great. It was even talked about on that PR. I just went in and tried to make that work, but there are a few ways to do it we should probably discuss first. All of this logic is static, so I would need static options. Not really a big deal, but that would only work for this particular ActivitySource which we happen to control fully. For other sources, living in external code, we should have an options solution for those too. I see two places. In our ActivityListener, or in our ActivityProcessor. I think from an API perspective, something like this would be great:

builder.AddActivitySource(HttpWebRequestActivitySource.ActivitySourceName, (options) => { options.ActivityStarted => (Activity httpActivity) => ...code to modify name... });

We would probably need to change to multiple listeners (instead of one) to make that work cleanly though?

pjanotti · 2020-05-29T22:29:19Z

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs

+
+            InstrumentRequest(request, activity);
+
+            activity.SetCustomProperty("HttpWebRequest.Request", request);


Q.: why the property is set even if not recording? Similar for other methods.

Short answer: No reason, just my gut feel for how it should be.

Long answer: The only reason to add these custom properties at all is just in case someone downstream needs to add or change the data based on the real objects. If I don't add them in all cases (like it is currently doing), they won't be able to change anything when recording in the limited mode. I felt like we needed to add them always (if we're going to add them at all) so if downstream consumers wanted to have limited/full logic, they could. At the cost of an allocation to store the objects:

https://github.com/dotnet/runtime/blob/bd4697644e7024d1e5be94b674f73066bee112a4/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L816

That is a good point. Perhaps we want to have some convention for the keys so child spans don't depend on a specific implementation handling the parent. I am not sure if this is necessary but we may want to provide some helper to "flatten" the search for the property: the child needs to navigate parents to search for this property.

pjanotti · 2020-05-29T22:30:25Z

src/OpenTelemetry.Instrumentation.Dependencies/OpenTelemetryBuilderExtensions.cs

+                throw new ArgumentNullException(nameof(builder));
+            }
+
+            GC.KeepAlive(HttpWebRequestActivitySource.Instance);


Q.: Why is this needed?

To be honest, not totally sure. That came from the original version in dotnet/runtime:

https://github.com/dotnet/runtime/blob/bd4697644e7024d1e5be94b674f73066bee112a4/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DiagnosticListener.cs#L40

I think it is basically a clever way of telling the runtime to initialize the static stuff in the HttpWebRequestActivitySource class. We need to touch some code for the runtime to do that, but we don't actually want to call anything. Doesn't really have anything to do with garbage collection AFAIK.

Thanks. It feels confusing because it references GC, perhaps it should be a method instead: Init() or Initialize() - anyway I think it is fine to address it later.

The Gc.Keepalive was part of the existing code, not introduced in this PR. Adding this note here and proceeding to merge this PR.

CodeBlanch · 2020-05-30T17:18:22Z

src/OpenTelemetry/Trace/Configuration/OpenTelemetrySdk.cs


                // The following parameter is not used now.
-                GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.AllData,
+                GetRequestedDataUsingParentId = (ref ActivityCreationOptions<string> options) => ActivityDataRequest.None, // Defer to parent whether or not we should record.


@cijothomas I think we had a bug here. Before we returned ActivityDataRequest.AllData which would turn on sampling for a child when its parent was not being sampled. The way I'm reading the source (https://github.com/dotnet/runtime/blob/bd4697644e7024d1e5be94b674f73066bee112a4/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L123-L126) we need to return less than the parent in order to preserve its sampling decision?

@tarekgh API is a bit confusing here. Maybe return should be null when you want to defer to the parent's current ActivityDataRequest mode? Also, only the Id is available from the parent, would we ever need the full parent Activity here?

/cc @MikeGoldsmith @pjanotti

Nevermind! Reverted the change. I realized writing a unit test for this that Activity w/ parent still flows through GetRequestedDataUsingContext. Once dotnet/runtime#37185 is available I'll add that test. Can't do it right now unless this code is changed to pass Activity.Current?.Context ?? default as parentContext when starting its Activity, which is what the PR is addressing. All that being said, we'll have Parent.ActivityTraceFlags at time of sampling (gets us Recorded), but don't we also need Parent.IsAllDataRequested? That's not on ActivityContext.

My comment that the API is a bit confusing still stands :)

@CodeBlanch

API is a bit confusing here. Maybe return should be null when you want to defer to the parent's current ActivityDataRequest mode? Also, only the Id is available from the parent, would we ever need the full parent Activity here?

The GetRequestedDataUsingParentId callback is called only when anyone calls Activity.StartActivity and passing the parent Id. Originally this callback requested by @noahfalk and will be used in the scenario in which the parent Id is enough to make the decision. I don't think the parent Activity object will be available in such scenarios because if it does, then GetRequestedDataUsingContext would be called instead. That is why the parent Activity is not used in such callbacks and we just use the Parent Id. The question now is, why you think the API is confusing? and what exactly the suggestion here? the listener has full control here to decide if interested in sampling such Activity with that parent Id or just ignore it and later decide about the child activities. why this is not enough?

@tarekgh Sorry this got long. I tried to make it "not a rambling mess" but the topic is big.

why you think the API is confusing?

Hard to say exactly! Sorry I know that isn't helpful :) We have the concept of "sampling" which I think people are naturally familiar with (somewhat?) but nothing in ActivitySource is really named along the lines of that. We have GetRequestedDataUsingParentId & GetRequestedDataUsingContext. Mental leap there to relate "GetRequestedData" to sampling and what we're trying to accomplish. I think just renaming these GetSamplingModeUsing... (GetSamplingLevel?) would help a lot. Maybe rename ActivityDataRequest -> ActivitySamplingMode. Just my $.02!

Here's an example of how hard it is to use this correctly:

Imagine a request comes into an application and an Activity is started. Sampler logic is run. It decides to sample at ActivityDataRequest.PropagationData level, for whatever reason. Now we go to call some dependency. A new Activity is created. Sampler looks at this new one. All it gets from the Parent (Activity.Current.Context) is ActivityTraceFlags, which will be ActivityTraceFlags.None in the case of ActivityDataRequest.PropagationData when Parent was sampled. What is a sampler to do here? It might think... the parent isn't recorded, nor should I be. Or it might think, I can decide to sample if I want to. But really it needs to know if ActivityContext != default than the parent was ActivityDataRequest.PropagationData and it should probably do the same for this child?

To fix that up I think we need to keep ActivityDataRequest on the Activity (or ActivityContext) and pass that in to GetRequestedDataUsing... for children. That will be maximum clarity.

Check out our current implementation:

opentelemetry-dotnet/src/OpenTelemetry/Trace/Configuration/OpenTelemetrySdk.cs

Lines 84 to 104 in dcedf02

GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) =>

{

var shouldSample = sampler.ShouldSample(

options.Parent,

options.Parent.TraceId,

default(ActivitySpanId), // Passing default SpanId here. The actual SpanId is not known before actual Activity creation

options.Name,

options.Kind,

options.Tags,

options.Links);

if (shouldSample.IsSampled)

{

return ActivityDataRequest.AllDataAndRecorded;

}

else

{

return ActivityDataRequest.None;

}

// TODO: Improve this to properly use ActivityDataRequest.AllData, PropagationData as well.

},

It doesn't have the rule for PropagationData so for my exercise above, we won't propagate the child. Bug?

It should be:

if (shouldSample.IsSampled) { return ActivityDataRequest.AllDataAndRecorded; } else if (options.Parent != default) { return ActivityDataRequest.PropagationData; } else { return ActivityDataRequest.None; }

No?

There seem to be two separate suggestions here:

Better name for RequestedData - like SamplingMode

Make the ActivityDataRequest of parent available to GetRequestedDataUsingContext callback, so child can know the exact parent decision - instead of just knowing ActivityContext

Both are relevant, but not tied to this PR alone - lets discuss in separate issue, and proceed with merging this PR.

(I can create issues or @CodeBlanch can - let me know)

cijothomas

Thanks!

Updated HttpWebRequestDiagnosticSource to use the new .NET 5 Activity…

8e92f13

…Source API.

CodeBlanch requested review from bruno-garcia, cijothomas, discostu105, lmolkova, MikeGoldsmith, pjanotti and SergeyKanzhelev as code owners May 22, 2020 05:53

CodeBlanch marked this pull request as draft May 22, 2020 05:54

CodeBlanch commented May 22, 2020

View reviewed changes

cijothomas reviewed May 22, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Show resolved Hide resolved

cijothomas reviewed May 22, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 22, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 22, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 22, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Outdated Show resolved Hide resolved

cijothomas reviewed May 22, 2020

View reviewed changes

cijothomas requested changes May 22, 2020

View reviewed changes

CodeBlanch added 4 commits May 22, 2020 09:57

Code review part 1 and string allocation reduction.

9702188

Code review part 2 added ExceptionInitializingInstrumentation event.

604e2a8

Code review feedback 3.

2c47326

Code review feedback 4.

8657a42

tarekgh reviewed May 22, 2020

View reviewed changes

Updated unit tests and some code review feedback.

ce44df6

CodeBlanch added 2 commits May 23, 2020 11:58

Merge branch 'master' into httpwebrequest-activitysource

cee5c86

Updated to JaegerActivityExporter since it was just merged.

e2e4e5e

CodeBlanch marked this pull request as ready for review May 23, 2020 19:03

CodeBlanch commented May 23, 2020

View reviewed changes

test/OpenTelemetry.Instrumentation.Dependencies.Tests/HttpWebRequestTests.Basic.net461.cs Show resolved Hide resolved

CodeBlanch commented May 23, 2020

View reviewed changes

test/OpenTelemetry.Instrumentation.Dependencies.Tests/HttpWebRequestTests.Basic.net461.cs Show resolved Hide resolved

CodeBlanch commented May 23, 2020

View reviewed changes

CodeBlanch and others added 2 commits May 23, 2020 13:03

Refactored http tag value caching so that it can be shared in depende…

05ef16c

…ncies project.

Merge branch 'master' into httpwebrequest-activitysource

47ea8fb

cijothomas reviewed May 27, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Show resolved Hide resolved

cijothomas reviewed May 27, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Show resolved Hide resolved

cijothomas reviewed May 27, 2020

View reviewed changes

...Telemetry.Instrumentation.Dependencies/Implementation/HttpWebRequestActivitySource.net461.cs Show resolved Hide resolved

CodeBlanch added 3 commits May 27, 2020 16:41

Code review.

3017828

Noticed a couple comments needed to be updated.

e0c388d

Added Activity.IsAllDataRequested logic.

b8638a7

MikeGoldsmith approved these changes May 29, 2020

View reviewed changes

pjanotti approved these changes May 29, 2020

View reviewed changes

CodeBlanch added 2 commits May 30, 2020 09:18

Merge branch 'master' into httpwebrequest-activitysource

911b5e7

Code review.

cb6ff06

CodeBlanch commented May 30, 2020

View reviewed changes

Reverted change to OpenTelemetrySdk.

e31c7d8

cijothomas approved these changes Jun 2, 2020

View reviewed changes

Merge branch 'master' into httpwebrequest-activitysource

515e3e1

cijothomas merged commit 96e1eb9 into open-telemetry:master Jun 2, 2020

CodeBlanch deleted the httpwebrequest-activitysource branch June 3, 2020 00:45

		}

		if (exception is WebException wexc && wexc.Response is HttpWebResponse response)


		InstrumentRequest(request, activity);

		activity.SetCustomProperty("request", request);

	private static void ProcessException(TelemetrySpan span, Exception exception)
	{
	if (exception is WebException wexc)
	{
	if (wexc.Response is HttpWebResponse response)
	{
	span.PutHttpStatusCode((int)response.StatusCode, response.StatusDescription);
	return;
	}

	switch (wexc.Status)
	{
	case WebExceptionStatus.Timeout:
	span.Status = Status.DeadlineExceeded;
	return;
	case WebExceptionStatus.NameResolutionFailure:
	span.Status = Status.InvalidArgument.WithDescription(exception.Message);
	return;
	case WebExceptionStatus.SendFailure:
	case WebExceptionStatus.ConnectFailure:
	case WebExceptionStatus.SecureChannelFailure:
	case WebExceptionStatus.TrustFailure:
	span.Status = Status.FailedPrecondition.WithDescription(exception.Message);
	return;
	case WebExceptionStatus.ServerProtocolViolation:
	span.Status = Status.Unimplemented.WithDescription(exception.Message);
	return;
	case WebExceptionStatus.RequestCanceled:
	span.Status = Status.Cancelled;
	return;
	case WebExceptionStatus.MessageLengthLimitExceeded:
	span.Status = Status.ResourceExhausted.WithDescription(exception.Message);
	return;
	}
	}

	span.Status = Status.Unknown.WithDescription(exception.Message);
	}
	}


		InstrumentRequest(request, activity);

		activity.SetCustomProperty("HttpWebRequest.Request", request);

	GetRequestedDataUsingContext = (ref ActivityCreationOptions<ActivityContext> options) =>
	{
	var shouldSample = sampler.ShouldSample(
	options.Parent,
	options.Parent.TraceId,
	default(ActivitySpanId), // Passing default SpanId here. The actual SpanId is not known before actual Activity creation
	options.Name,
	options.Kind,
	options.Tags,
	options.Links);
	if (shouldSample.IsSampled)
	{
	return ActivityDataRequest.AllDataAndRecorded;
	}
	else
	{
	return ActivityDataRequest.None;
	}

	// TODO: Improve this to properly use ActivityDataRequest.AllData, PropagationData as well.
	},

HttpWebRequest ActivitySource #694

HttpWebRequest ActivitySource #694

Conversation

CodeBlanch commented May 22, 2020 • edited Loading

Changes

Checklist

CodeBlanch commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

tarekgh May 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented May 28, 2020

MikeGoldsmith left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarekgh May 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

CodeBlanch commented May 22, 2020 •

edited

Loading

tarekgh May 22, 2020 •

edited

Loading

tarekgh May 31, 2020 •

edited

Loading