Migrate to ActivitySource #30089

shirhatti · 2021-02-11T07:47:31Z

Current behavior:

if (loggingEnabled || diagnosticsListenerEnabled), new up Activity
activity.SetParentId(headers["traceparent"] or headers["request-id"])
activity Add Baggage from headers["baggage"] or headers["Correlation-Context"]
_diagnosticsListener.OnActivityImport(activity, httpContext)
activity.Start()
_diagnosticsListener.Write(ActivityStartKey, httpContext)

New behavior:

if (loggingEnabled || diagnosticsListenerEnabled), new up a dummyListener
Defer creation of Activity to ActivitySource

if (activity is not null)
{

Add baggage
_diagnosticsListener.OnActivityImport(activity, httpContext)
_diagnosticListener.Write(ActivityStartKey, httpContext);

}

Questions:

Baggage isn't present on Activity before starting
Is it okay if we don't fire diagnosticsListener.OnActivityImport? Is ordering important? Can I do it after Activity.Start()?

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

tarekgh · 2021-02-12T17:34:01Z

CC @noahfalk

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

shirhatti · 2021-02-16T06:02:03Z

FYI @pakrym

pakrym · 2021-02-19T16:44:37Z

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


            var headers = httpContext.Request.Headers;
            if (!headers.TryGetValue(HeaderNames.TraceParent, out var requestId))
            {
                headers.TryGetValue(HeaderNames.RequestId, out requestId);
            }
-
-            if (!StringValues.IsNullOrEmpty(requestId))
+            var activity = _activitySource.StartActivity(ActivityName, ActivityKind.Server, requestId);


@tarekgh/@noahfalk, what's the expected format of the activity name when used with the ActivitySource?

I thought that considering the ActivitySource name is Microsoft.AspNetCore.Hosting the activity name should be HttpRequestIn and not Microsoft.AspNetCore.Hosting.HttpRequestIn

You are correct @pakrym, the Activity names should be only the instrumented operation name (i.e. HttpRequestIn for this specific code). And ActivitySource name should reflect the company name, component name, and the whole instrumented area (i.e. Microsoft.AspNetCore.Hosting).

As this code existed for a while, do you think there will be any issue changing the Activity name?

I think that's another area where reusing the same activity doesn't work well. We can't change existing activity names and ActivitySource-activity should have a different one.

We spoke about this earlier today, the Activity name would have to be Microsoft.AspNetCore.Hosting.HttpRequestIn to preserve compat. Instrumentation libraries can set Activity.DisplayName if they would prefer something more concise.

Should we say there is no guidance around Activity.Name then? If none of the Microsoft libraries would support it.

The guidelines for Activity.Name still hold we only relax it for the sake of the compatibility. Any new written code with no compatibility concern should apply the guidelines.

But if all the consuming code (OT) would forever have to check for legacy behavior. What's the point of the guideline?

I am hoping the OTel will get rid of listening to the legacy behavior. My understanding is after this change, they are listening to the legacy for the payload and not the Activity itself. This may be the next step to look at that and try help OTel to get rid of this legacy listening.

I am hoping the OTel will get rid of listening to the legacy behavior

After this change, all ASP.NET Core ActivitySource activities would forever have legacy Activity.Name.
Same for Azure SDKs if we try to reuse the same activity for both.

As I mentioned we relaxed the guidelines for the Activity.Name for the sake of compatibility. If anyone writing any new code with no compatibility concern, would better to follow the guidelines. For aspnet case, I suggest setting the display name too.

shirhatti · 2021-02-23T05:29:27Z

Waiting on dotnet/runtime#48374 for the next update

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

src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs

shirhatti · 2021-02-23T22:34:23Z

Before: crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/json.benchmarks.yml --scenario json --profile aspnet-perf-lin --application.environmentVariables ASPNETCORE_LogLevel=Error --application.framework net6.0 --application.options.displayOutput true

| application           |                           |
| --------------------- | ------------------------- |
| CPU Usage (%)         | 97                        |
| Cores usage (%)       | 1,164                     |
| Working Set (MB)      | 180                       |
| Build Time (ms)       | 4,726                     |
| Start Time (ms)       | 207                       |
| Published Size (KB)   | 280,235                   |
| .NET Core SDK Version | 6.0.100-preview.3.21123.4 |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 50        |
| Cores usage (%)        | 594       |
| Working Set (MB)       | 38        |
| Start Time (ms)        | 0         |
| First Request (ms)     | 127       |
| Requests/sec           | 380,244   |
| Requests               | 5,741,726 |
| Mean latency (ms)      | 0.84      |
| Max latency (ms)       | 52.46     |
| Bad responses          | 0         |
| Socket errors          | 0         |
| Read throughput (MB/s) | 55.12     |
| Latency 50th (ms)      | 0.65      |
| Latency 75th (ms)      | 0.73      |
| Latency 90th (ms)      | 0.82      |
| Latency 99th (ms)      | 8.60      |

After: crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/json.benchmarks.yml --scenario json --profile aspnet-perf-lin --application.environmentVariables ASPNETCORE_LogLevel=Error --application.options.outputFiles C:\Users\shirh\source\repos\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Hosting\Release\net6.0\Microsoft.AspNetCore.Hosting.dll --application.framework net6.0 --application.options.displayOutput true

| application           |                           |
| --------------------- | ------------------------- |
| CPU Usage (%)         | 97                        |
| Cores usage (%)       | 1,164                     |
| Working Set (MB)      | 179                       |
| Build Time (ms)       | 12,768                    |
| Start Time (ms)       | 217                       |
| Published Size (KB)   | 280,235                   |
| .NET Core SDK Version | 6.0.100-preview.3.21123.4 |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 51        |
| Cores usage (%)        | 611       |
| Working Set (MB)       | 48        |
| Build Time (ms)        | 7,868     |
| Start Time (ms)        | 0         |
| Published Size (KB)    | 76,407    |
| .NET Core SDK Version  | 3.1.406   |
| First Request (ms)     | 126       |
| Requests/sec           | 397,795   |
| Requests               | 6,006,528 |
| Mean latency (ms)      | 0.78      |
| Max latency (ms)       | 40.68     |
| Bad responses          | 0         |
| Socket errors          | 0         |
| Read throughput (MB/s) | 57.66     |
| Latency 50th (ms)      | 0.62      |
| Latency 75th (ms)      | 0.70      |
| Latency 90th (ms)      | 0.79      |
| Latency 99th (ms)      | 7.72      |

@Tratcher @sebastienros Is there a better benchmark? I remember y'all mentioning there is a YARP benchmark for which Activity creation in hosting was on the hot path?

sebastienros · 2021-02-23T22:38:00Z

In the scenarios page, look for yarp. Beware that the sample command is for httpclient though.

shirhatti · 2021-02-23T22:48:32Z

Before: crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/proxy.benchmarks.yml --scenario proxy-httpclient --profile aspnet-perf-lin --application.framework net6.0 --application.options.displayOutput true

| downstream            |         |
| --------------------- | ------- |
| CPU Usage (%)         | 87      |
| Cores usage (%)       | 1,043   |
| Working Set (MB)      | 149     |
| Build Time (ms)       | 4,180   |
| Start Time (ms)       | 287     |
| Published Size (KB)   | 95,075  |
| .NET Core SDK Version | 3.1.406 |


| application           |                           |
| --------------------- | ------------------------- |
| CPU Usage (%)         | 95                        |
| Cores usage (%)       | 1,136                     |
| Working Set (MB)      | 189                       |
| Build Time (ms)       | 4,142                     |
| Start Time (ms)       | 160                       |
| Published Size (KB)   | 263,567                   |
| .NET Core SDK Version | 6.0.100-preview.3.21123.4 |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 40        |
| Cores usage (%)        | 475       |
| Working Set (MB)       | 42        |
| Start Time (ms)        | 111       |
| First Request (ms)     | 213       |
| Requests               | 1,878,924 |
| Bad responses          | 0         |
| Mean latency (us)      | 2,038     |
| Max latency (us)       | 51,858    |
| Requests/sec           | 125,322   |
| Requests/sec (max)     | 136,375   |
| Read throughput (MB/s) | 14.45     |

After: crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/proxy.benchmarks.yml --scenario proxy-httpclient --profile aspnet-perf-lin --application.options.outputFiles C:\Users\shirh\source\repos\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Hosting\Release\net6.0\Microsoft.AspNetCore.Hosting.dll --application.framework net6.0 --application.options.displayOutput true

| downstream            |         |
| --------------------- | ------- |
| CPU Usage (%)         | 86      |
| Cores usage (%)       | 1,038   |
| Working Set (MB)      | 148     |
| Build Time (ms)       | 5,190   |
| Start Time (ms)       | 288     |
| Published Size (KB)   | 95,075  |
| .NET Core SDK Version | 3.1.406 |


| application           |                           |
| --------------------- | ------------------------- |
| CPU Usage (%)         | 95                        |
| Cores usage (%)       | 1,137                     |
| Working Set (MB)      | 186                       |
| Build Time (ms)       | 5,000                     |
| Start Time (ms)       | 170                       |
| Published Size (KB)   | 263,567                   |
| .NET Core SDK Version | 6.0.100-preview.3.21123.4 |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 40        |
| Cores usage (%)        | 476       |
| Working Set (MB)       | 52        |
| Build Time (ms)        | 5,820     |
| Start Time (ms)        | 111       |
| Published Size (KB)    | 77,083    |
| .NET Core SDK Version  | 3.1.406   |
| First Request (ms)     | 207       |
| Requests               | 1,857,181 |
| Bad responses          | 0         |
| Mean latency (us)      | 2,062     |
| Max latency (us)       | 50,195    |
| Requests/sec           | 124,134   |
| Requests/sec (max)     | 138,900   |
| Read throughput (MB/s) | 14.29     |

sebastienros · 2021-02-23T22:53:20Z

--scenario proxy-httpclient this is not yarp, this is a generic httpclient proxy without logging. Use --scenario proxy-yarp intead

sebastienros · 2021-02-23T22:54:26Z

Another tip. You can save the results with --output old.json, and --output new.json then do crank compare old.json new.json and get a nice comparison table to share

shirhatti · 2021-02-23T23:01:16Z

Old:

crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/proxy.benchmarks.yml --scenario proxy-yarp --profile aspnet-perf-lin --application.framework net6.0 --output old.json

New:

crank.exe --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/proxy.benchmarks.yml --scenario proxy-yarp --profile aspnet-perf-lin --application.options.outputFiles C:\Users\shirh\source\repos\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Hosting\Release\net6.0\Microsoft.AspNetCore.Hosting.dll --application.framework net6.0

| downstream            | old     | new     |         |
| --------------------- | ------- | ------- | ------- |
| CPU Usage (%)         |      85 |      82 |  -3.53% |
| Cores usage (%)       |   1,014 |     984 |  -2.96% |
| Working Set (MB)      |     149 |     149 |   0.00% |
| Build Time (ms)       |   5,219 |   4,243 | -18.70% |
| Start Time (ms)       |     295 |     293 |  -0.68% |
| Published Size (KB)   |  95,075 |  95,075 |   0.00% |
| .NET Core SDK Version | 3.1.406 | 3.1.406 |         |


| application           | old                       | new                       |         |
| --------------------- | ------------------------- | ------------------------- | ------- |
| CPU Usage (%)         |                        96 |                        97 |  +1.04% |
| Cores usage (%)       |                     1,154 |                     1,160 |  +0.52% |
| Working Set (MB)      |                       212 |                       212 |   0.00% |
| Build Time (ms)       |                     7,003 |                     5,355 | -23.53% |
| Start Time (ms)       |                       367 |                       352 |  -4.09% |
| Published Size (KB)   |                   263,844 |                   263,844 |   0.00% |
| .NET Core SDK Version | 6.0.100-preview.3.21123.4 | 6.0.100-preview.3.21123.4 |         |


| load                   | old       | new       |         |
| ---------------------- | --------- | --------- | ------- |
| CPU Usage (%)          |        35 |        35 |   0.00% |
| Cores usage (%)        |       417 |       417 |   0.00% |
| Working Set (MB)       |        42 |        42 |   0.00% |
| Start Time (ms)        |       111 |       110 |  -0.90% |
| First Request (ms)     |       246 |       239 |  -2.85% |
| Requests               | 1,443,811 | 1,426,715 |  -1.18% |
| Bad responses          |         0 |         0 |         |
| Mean latency (us)      |     2,653 |     2,683 |  +1.13% |
| Max latency (us)       |    53,336 |    72,823 | +36.54% |
| Requests/sec           |    96,441 |    95,419 |  -1.06% |
| Requests/sec (max)     |   105,120 |   104,774 |  -0.33% |
| Read throughput (MB/s) |     11.11 |     10.98 |  -1.18% |

tarekgh · 2021-02-24T03:51:39Z

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

        {
-            var activity = new Activity(ActivityName);
+            var activity = _activitySource.CreateActivity(ActivityName, ActivityKind.Server);


var [](start = 12, length = 3)

does it make sense to check _activitySource.HasListeners() before calling _activitySource.CreateActivity if you want save some cycles when there is no listeners?

I don't think it'll make a difference. You already do the check in ActivitySource.CreateActivity- https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L172

davidfowl · 2021-02-24T06:27:00Z

allocation profile plz 😄

shirhatti · 2021-02-24T20:38:59Z

(new allocation profile on the left, old one of the right)

No difference from eyeballing it.

The only thing mildly noteworthy is that average Activity size went up from 176 to 192 bytes

ghost · 2021-02-25T02:11:30Z

Hello @shirhatti!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

shirhatti requested a review from Tratcher as a code owner February 11, 2021 07:47

ghost added the area-servers label Feb 11, 2021

shirhatti marked this pull request as draft February 11, 2021 07:48

shirhatti linked an issue Feb 11, 2021 that may be closed by this pull request

Migrate to use ActivitySource API to create Activities in AspNetCore Hosting #29132

Closed

tarekgh reviewed Feb 11, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Outdated Show resolved Hide resolved

tarekgh reviewed Feb 11, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Outdated Show resolved Hide resolved

tarekgh reviewed Feb 11, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Outdated Show resolved Hide resolved

tarekgh reviewed Feb 12, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Outdated Show resolved Hide resolved

shirhatti mentioned this pull request Feb 12, 2021

Add API to allow forcing creation of a Activity using ActivitySource (rather than relying on ActivityListener) dotnet/runtime#48237

Closed

noahfalk reviewed Feb 13, 2021

View reviewed changes

pakrym reviewed Feb 19, 2021

View reviewed changes

shirhatti added 4 commits February 23, 2021 13:24

WIP: Migrate to ActivitySource

0f10c14

Add ActivityListener test

8ce8411

Fix remaining tests

d3ff481

Remove dummy listener

224912b

shirhatti force-pushed the shirhatti/activitysource branch from b1faee7 to 224912b Compare February 23, 2021 21:33

tarekgh reviewed Feb 23, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Outdated Show resolved Hide resolved

Use ActivitySource.CreateActivity APIs

fe50c4b

shirhatti commented Feb 23, 2021

View reviewed changes

src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs Show resolved Hide resolved

shirhatti added 2 commits February 23, 2021 14:08

Undo changes to Activity.Stop

b2b8ee6

Undo test change

606da28

shirhatti commented Feb 23, 2021

View reviewed changes

src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs Show resolved Hide resolved

shirhatti marked this pull request as ready for review February 23, 2021 22:12

shirhatti requested a review from tarekgh February 23, 2021 22:14

tarekgh reviewed Feb 24, 2021

View reviewed changes

tarekgh approved these changes Feb 24, 2021

View reviewed changes

shirhatti added the auto-merge label Feb 25, 2021

ghost merged commit 8df6490 into main Feb 25, 2021

ghost deleted the shirhatti/activitysource branch February 25, 2021 02:11

MihaZupan mentioned this pull request Jun 18, 2021

Add ActivitySource support to DiagnosticsHandler dotnet/runtime#54437

Merged

ManickaP mentioned this pull request Feb 3, 2022

Add ActivitySource support to DiagnosticsHandler dotnet/runtime#64753

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to ActivitySource #30089

Migrate to ActivitySource #30089

shirhatti commented Feb 11, 2021 •

edited

Loading

tarekgh commented Feb 12, 2021

shirhatti commented Feb 16, 2021

pakrym Feb 19, 2021

tarekgh Feb 19, 2021

pakrym Feb 19, 2021

shirhatti Feb 23, 2021 •

edited

Loading

pakrym Feb 23, 2021

tarekgh Feb 23, 2021

pakrym Feb 23, 2021

tarekgh Feb 23, 2021

pakrym Feb 23, 2021 •

edited

Loading

tarekgh Feb 23, 2021 •

edited

Loading

shirhatti commented Feb 23, 2021

shirhatti commented Feb 23, 2021 •

edited

Loading

sebastienros commented Feb 23, 2021

shirhatti commented Feb 23, 2021

sebastienros commented Feb 23, 2021

sebastienros commented Feb 23, 2021

shirhatti commented Feb 23, 2021

tarekgh Feb 24, 2021

shirhatti Feb 24, 2021

davidfowl commented Feb 24, 2021

shirhatti commented Feb 24, 2021 •

edited

Loading

ghost commented Feb 25, 2021

Migrate to ActivitySource #30089

Migrate to ActivitySource #30089

Conversation

shirhatti commented Feb 11, 2021 • edited Loading

tarekgh commented Feb 12, 2021

shirhatti commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirhatti Feb 23, 2021 • 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

pakrym Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

tarekgh Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

shirhatti commented Feb 23, 2021

shirhatti commented Feb 23, 2021 • edited Loading

sebastienros commented Feb 23, 2021

shirhatti commented Feb 23, 2021

sebastienros commented Feb 23, 2021

sebastienros commented Feb 23, 2021

shirhatti commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Feb 24, 2021

shirhatti commented Feb 24, 2021 • edited Loading

ghost commented Feb 25, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

shirhatti commented Feb 11, 2021 •

edited

Loading

shirhatti Feb 23, 2021 •

edited

Loading

pakrym Feb 23, 2021 •

edited

Loading

tarekgh Feb 23, 2021 •

edited

Loading

shirhatti commented Feb 23, 2021 •

edited

Loading

shirhatti commented Feb 24, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.