Asp.Net Core Requests Instrumentation - change to use new Activity API #692

cijothomas · 2020-05-21T20:34:51Z

Goal: Fixes 3-d from #684

This is very draft PR to discuss with community/.NET team about the issues with adapting "old" with "new" DiagnosticSource instrumentation.

"old" - means instrumentation using DiagnosticSource.StartActivity() - currently implemented by Asp.Net Core, SqlClient, HttpClient .Net Core etc.

"new" means instrumentation using ActivitySource.StartActivity().

As all the existing libraries are implemening the old instrumentation method, we need adapters to translate them into new way.
Conceptually, it involves the following:

Listen/Subscribe to old events using DiagnosticListeners.AllListeners.Subscrive.
Use reflection to extract span properties from payload.
Populate them into Activity.Tags.

Issues that need to be addressed:

The activity which is coming from the old instrumentation will have ActivityKind set to Internal (which is the default). This property is not mutable after activity start.
The activity from old instrumentation will not have any knowledge about sampling, and activity is always created and has activity.IsAllDataRequested = true all the time. The adapters will now need to call Samplers, and take action based on sampling outcome. But to do this, adapters will have to take dependency on OT SDK. (samplers are not part of API).

One proposal which solves the above 2 issues (but causes something else) is added in this PR. The proposal is:
Adapters keep listening to the old events in the current way.
Inside OnStart and OnEnd callbacks, adapters create a new activity using ActivitySource, and populate its tags. This new activity will conform to new usage pattern.

This obviously has the drawback of creating an additional activity. (bad perf , how bad I need to measure)

Opening this draft PR early to solicit feedback on this approach and alternates.

…3adapter1a

cijothomas · 2020-05-21T20:36:07Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -60,11 +63,21 @@ public override void OnStartActivity(Activity activity, object payload)
                return;
            }

+            // Create a brand new activity using ActivitySource


I added this code into existing place itself, which does span population. Once we settle on approach, we'll be removing all the span populations and replace with activity.

cijothomas · 2020-05-21T20:36:52Z

samples/Exporters/Web/Startup.cs

+
+            OpenTelemetrySdk.EnableOpenTelemetry(
+                (builder) => builder.AddActivitySource("Microsoft.AspNetCore")
+                .UseConsoleActivityExporter(opt => opt.DisplayAsJson = opt.DisplayAsJson));


for easily trying this out locally, i changed to use consoleexporter.
The Instrumention(Adapter) hook to OpenTelemetryBuilder is not implemented in this draft PR.

tarekgh · 2020-05-21T20:44:32Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -90,6 +103,25 @@ public override void OnStartActivity(Activity activity, object payload)
                span.PutHttpUserAgentAttribute(userAgent);
                span.PutHttpRawUrlAttribute(GetUri(request));
            }
+
+            if (newActivity.IsAllDataRequested)


newActivity [](start = 16, length = 11)

this can be null

tarekgh · 2020-05-21T20:45:21Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

+            {
+                newActivity.DisplayName = path;
+            }
+


you can write this block as

newActivity?.DisplayName = path;

Gets this error. Need a newer c# version for it to work?

add

<LangVersion>7.3</LangVersion>

to the csproj

tarekgh · 2020-05-21T20:48:25Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -36,6 +36,9 @@ internal class HttpInListener : ListenerHandler
        private readonly bool hostingSupportsW3C = false;
        private readonly AspNetCoreInstrumentationOptions options;

+        // Create an ActivitySource here to re-create Activity.
+        private ActivitySource activitySource = new ActivitySource("Microsoft.AspNetCore");


ActivitySource [](start = 16, length = 14)

do we usually create only one instance of HttpInListener and use it the life time of the app/service? I am asking to know if we need to explicitly dispose this ActivitySource when it is not needed.

Yes only one instance for life of app.

good. we should be ok here then. thanks.

In the existing implementation, we dispose the DiagnosticListeners when TracerFactory is disposed. Don't think we need it here.

tarekgh · 2020-05-21T20:53:33Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -114,8 +146,15 @@ public override void OnStopActivity(Activity activity, object payload)
                var response = context.Response;

                span.PutHttpStatusCode(response.StatusCode, response.HttpContext.Features.Get<IHttpResponseFeature>().ReasonPhrase);
+
+                // activity here will be the activity which we created using ActivitySource in the Start callback.


// activity here will be the activity which we created using ActivitySource in the Start callback. [](start = 15, length = 99)

how this magic is done? doesn't OnStopActivity is teh call back called from DiagnosticListener?

diagnosticlistener callback itself doesn't give us activity. Instead we access it using Activity.Current.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Instrumentation/DiagnosticSourceListener.cs#L40

Since we started a new activity() in the OnBegin, Activity.Current will now be the one we created in OnBegin. This will have its .Parent set to the one created by "old" instrumentation code using DiagnosticSource.StartActivity

make sense. thanks for the clarification. I am not expecting DiagnsoticListener will notify on the stop event of the Activity created from ActivitySource, right?

Correct. (In fact diagnosticlistener will only notify for diagnosticsource.write events, never for activity.start or stop. On listening side, we get Activity.Current to get the activity)

CodeBlanch · 2020-05-22T00:52:09Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

@@ -90,6 +103,28 @@ public override void OnStartActivity(Activity activity, object payload)
                span.PutHttpUserAgentAttribute(userAgent);
                span.PutHttpRawUrlAttribute(GetUri(request));
            }
+
+            if (newActivity != null)


nit: Can probably combine these two if blocks.

CodeBlanch · 2020-05-22T01:09:29Z

@cijothomas I'm trying to fully understand the second issue in the description. If I'm following correctly, our new Activity could have IsAllDataRequested=false because the ActivityListener in OpenTelemetrySdk.EnableOpenTelemetry will test the sampler via GetRequestedDataUsingContext and turn off collection if needed. We can't do that to the old Activity because 1) the property isn't "settable", 2) we no longer have Span as a go-between, and 3) the old Activity doesn't fire ActivityListener like the new one?

If that is correct, could we possibly have the .NET guys flow the old Activity through the new ActivityListener so we have a chance to play with the data in both cases? I'm sure there are a bunch of reasons why it isn't possible but I thought I'd ask!

cijothomas · 2020-05-22T01:54:53Z

@cijothomas I'm trying to fully understand the second issue in the description. If I'm following correctly, our new Activity could have IsAllDataRequested=false because the ActivityListener in OpenTelemetrySdk.EnableOpenTelemetry will test the sampler via GetRequestedDataUsingContext and turn off collection if needed. We can't do that to the old Activity because 1) the property isn't "settable", 2) we no longer have Span as a go-between, and 3) the old Activity doesn't fire ActivityListener like the new one?

If that is correct, could we possibly have the .NET guys flow the old Activity through the new ActivityListener so we have a chance to play with the data in both cases? I'm sure there are a bunch of reasons why it isn't possible but I thought I'd ask!

The IsAllDataRequested is settable - and the adapter itself can call Sampler and set IsAllDataRequested. But this requires the adapter to have OT SDK reference, something which we'd like to avoid.

The old Activity too fires ActivityListener - Its fired if we enable ActivityListener for it. The old Activity has "empty" ActivitySource name, so it can be enabled with a call like builder.AddActivitySource(string.Empty). My alternate approach was using it. It was something like:

Enable DiagnosticSource listener as before.
Enable ActivityListener too for the old Activity. (builder.AddActivitySource(string.Empty))
We now get both callbacks, one from DiagnosticListener and one from Activitylistener.
In the callback from DiagnosticListener, enrich the activity from payload. like add activity.AddTag...
This approach also faces the issue of activity.ActivityKind not settable.
Also requires us to do sampling calls inside this callback - again adding OT SDK dependency for adapter.
The callback from ActivityListener works now gets the activity populated with all the tags and other things.

Let me know if this makes sense. (I am still exploring this more, so may have additional insights later :))

pjanotti · 2020-05-22T04:37:29Z

@tarekgh any issue with making Activity.Kind settable?

@cijothomas what is the desired benefit of avoiding the OpenTelemetry.Sdk? For symmetry with the API/SDK we likely should allow injection of samplers in some form.

pjanotti · 2020-05-22T04:47:04Z

Perhaps we could wrap the DiagnosticListener of each instrumentation with a common/base one that takes care of the "sampling" issue before passing the activity to the instrumentation one. So instrumentation writers don't need to worry about that and we have common code to deal with the SDK.

cijothomas · 2020-05-22T04:49:28Z

@tarekgh any issue with making Activity.Kind settable?

@cijothomas what is the desired benefit of avoiding the OpenTelemetry.Sdk? For symmetric with the API/SDK we likely should allow injection of samplers in some form.

I was chatting with Tarek yesterday about making Kind settable. I believe the API Spec says Kind must be immutable after starting. (We did a quick search on spec about this, but didn't find that restriction, but we both recall seeing it somewhere)

I was referring to the fact that instrumentationadapters will need to take dependency on OT.SDK. which can be avoided. ( I know our current instrumentation has SDK dependency, but @SergeyKanzhelev and I was trying to get rid of that dependency, and have instrumetation depend on only the API, which in our "new" case would be just DiagnosticSource

cijothomas · 2020-05-22T04:50:43Z

Perhaps we could wrap the DiagnosticListener of each instrumentation with a common/base one that takes care of the "sampling" issue before passing the activity to the instrumentation one. So instrumentation writers don't need to worry about that and we have common code to deal with the SDK.

Yup - once Activity.Kind issue is handled, this approach is what I am going to explore more. I tried this first :) (before the duplicate activity creation method proposed in this PR)

cijothomas · 2020-05-22T04:56:59Z

Perhaps we could wrap the DiagnosticListener of each instrumentation with a common/base one that takes care of the "sampling" issue before passing the activity to the instrumentation one. So instrumentation writers don't need to worry about that and we have common code to deal with the SDK.

Yup - once Activity.Kind issue is handled, this approach is what I am going to explore more. I tried this first :) (before the duplicate activity creation method proposed in this PR)

Will send another PR with the alternate proposal. Will use reflection hack to set Kind temporarily.

pjanotti · 2020-05-22T06:15:04Z

I was chatting with Tarek yesterday about making Kind settable. I believe the API Spec says Kind must be immutable after starting. (We did a quick search on spec about this, but didn't find that restriction, but we both recall seeing it somewhere)

I took a look at this and it seems that Kind was always settable, later it was added as a parameter to span creation on the specs. Java has a setter for it since part of the spec was derived from its implementation and it existed before Kind being added to the creation of the span. Go doesn't but it seems that it is just because they added the Span.Kind when the specs added the bit about Kind being an optional argument when creating the span. We can double-check with spec folks but we should be good in this regard.

cijothomas · 2020-05-22T14:08:39Z

I was chatting with Tarek yesterday about making Kind settable. I believe the API Spec says Kind must be immutable after starting. (We did a quick search on spec about this, but didn't find that restriction, but we both recall seeing it somewhere)

I took a look at this and it seems that Kind was always settable, later it was added as a parameter to span creation on the specs. Java has a setter for it since part of the spec was derived from its implementation and it existed before Kind being added to the creation of the span. Go doesn't but it seems that it is just because they added the Span.Kind when the specs added the bit about Kind being an optional argument when creating the span. We can double-check with spec folks but we should be good in this regard.

Okay. If Kind is settable, then @tarekgh can help to get this addressed for next .NET preview.

tarekgh · 2020-05-22T19:04:27Z

@pjanotti @cijothomas I recall in early discussions when I proposed the first draft of the Activity APIs, I had the Kind was settable. I recall I got some feedback which I don't recall from who exactly mentioning the Kind should not be settable after creating the Activity. I don't mind to make it settable again if no objection from anyone.

CC @reyang @noahfalk

MikeGoldsmith · 2020-05-24T14:41:32Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

+                {
+                    if (request.Host.Port == 80 || request.Host.Port == 443)
+                    {
+                        newActivity.AddTag("http.host", request.Host.Host);


Why do you not include the port for 80/443?

just retaining the current behavior. Current behavior is probably because 80,443 are common and can be deducted from other info like url etc.

MikeGoldsmith · 2020-05-24T14:46:35Z

src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

+                    newActivity.AddTag("http.path", path);
+
+                    var userAgent = request.Headers["User-Agent"].FirstOrDefault();
+                    newActivity.AddTag("http.user_agent", userAgent);


It's possible for FirstOrDefault to return null so we should add null check too.

Suggested change

newActivity.AddTag("http.user_agent", userAgent);

if (!string.IsNullOrEmpty(userAgent))

{

newActivity.AddTag("http.user_agent", userAgent);

}

cijothomas · 2020-06-02T13:47:15Z

Closing this in favor of the alternate proposal.

cijothomas added 3 commits May 20, 2020 10:58

trying out modifying httpinlistener to transform Activity to new type

24a43fe

Merge remote-tracking branch 'origin/master' into cijothomas/activity…

b2e1088

…3adapter1a

modify httpin to create a duplicate activity using ActivitySource

58be52b

cijothomas commented May 21, 2020

View reviewed changes

tarekgh reviewed May 21, 2020

View reviewed changes

add null check for activity

597fda7

CodeBlanch reviewed May 22, 2020

View reviewed changes

MikeGoldsmith approved these changes May 24, 2020

View reviewed changes

cijothomas mentioned this pull request May 26, 2020

Instrumentation Adapters to support Activity API #701

Merged

Merge branch 'master' into cijothomas/activity3adapter1a

c5a94cd

cijothomas closed this Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asp.Net Core Requests Instrumentation - change to use new Activity API #692

Asp.Net Core Requests Instrumentation - change to use new Activity API #692

cijothomas commented May 21, 2020

cijothomas May 21, 2020

cijothomas May 21, 2020

tarekgh May 21, 2020

tarekgh May 21, 2020

cijothomas May 21, 2020

tarekgh May 21, 2020 •

edited

Loading

tarekgh May 21, 2020

cijothomas May 21, 2020

tarekgh May 21, 2020

cijothomas May 22, 2020

tarekgh May 21, 2020

cijothomas May 21, 2020

tarekgh May 21, 2020

cijothomas May 22, 2020

CodeBlanch May 22, 2020

CodeBlanch commented May 22, 2020

cijothomas commented May 22, 2020

pjanotti commented May 22, 2020 •

edited

Loading

pjanotti commented May 22, 2020

cijothomas commented May 22, 2020

cijothomas commented May 22, 2020

cijothomas commented May 22, 2020

pjanotti commented May 22, 2020

cijothomas commented May 22, 2020

tarekgh commented May 22, 2020

MikeGoldsmith May 24, 2020

cijothomas May 26, 2020

MikeGoldsmith May 24, 2020

cijothomas commented Jun 2, 2020

Asp.Net Core Requests Instrumentation - change to use new Activity API #692

Asp.Net Core Requests Instrumentation - change to use new Activity API #692

Conversation

cijothomas commented May 21, 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

tarekgh May 21, 2020 • edited Loading

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 22, 2020

cijothomas commented May 22, 2020

pjanotti commented May 22, 2020 • edited Loading

pjanotti commented May 22, 2020

cijothomas commented May 22, 2020

cijothomas commented May 22, 2020

cijothomas commented May 22, 2020

pjanotti commented May 22, 2020

cijothomas commented May 22, 2020

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

cijothomas commented Jun 2, 2020

tarekgh May 21, 2020 •

edited

Loading

pjanotti commented May 22, 2020 •

edited

Loading