Use DiagnosticSource to instrument ASP.NET Core #611

lucaspimentel · 2020-01-22T17:47:18Z

This PR rewrites the ASP.NET Core integration to subscribe to the built-in DiagnosticSource events instead of replacing method calls.

src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnostics.cs

src/Datadog.Trace/DiagnosticListeners/PropertyFetcher.cs

src/Datadog.Trace/DiagnosticListeners/HostingEventProcessor.cs

…e the startup hook is executed on ASP.NET Core

…apture exceptions in ASP.NET Core (not MVC)

… now have Microsoft.AspNetCore.Diagnostics.UnhandledException

colin-higgins · 2020-02-03T15:38:38Z

src/Datadog.Trace.ClrProfiler.Managed/Datadog.Trace.ClrProfiler.Managed.csproj

@@ -7,10 +7,12 @@
    <Version>1.11.1-prerelease</Version>
    <Title>Datadog APM - ClrProfiler</Title>
    <Description>OBSOLETE. This package is deprecated and exists only for backwards compatibility.


If this package is obsolete, should we continue releasing new versions?

Mostly I just wanted to update the description text. It causes confusion among users. For example, see #609, #619, and some conversations on public Slack.

But we may need to keep updating it anyway to keep the version in sync with the installers for users who haven't realized that they should remove the package.

(edit: grammar)

If they are upgrading, wouldn't that be when we want them to stop including it?

Ideally, yes, but we don't have a way from stopping a user from upgrading blindly without reading the new package description. And if a user updates the installer and keep the older NuGet package (because we stop updating it), then the non-matching versions causes other issues.

I think it's little effort for us to keep bumping the version here for now so we don't break users who are using this package. We can drop it entirely when we make other big breaking changes (2.0?).

Keep in mind that users were using this packages for 3 different use cases:

manual instrumentation (because it includes Datadog.Trace)

ASP.NET integration (before it moves to Datadog.Trace.AspNet)

auto-instrumentation on .NET Core (before the assemblies were moved into the installer)

colin-higgins · 2020-02-03T17:29:48Z

src/Datadog.Trace/DiagnosticListeners/DiagnosticObserver.cs

+
+        public virtual bool IsSubscriberEnabled()
+        {
+            return true;


Any plans to make this config based?

We should! This is left-over from the project I based these changes on. I can remove it or we can leave it as a placeholder for "future feature". I have no strong feelings either way.

…sed very often)

… tests

…erly validate the http status code and http method, and fix the tags for http status code and http method

…n.props

* clean up env vars, project properties, and unused references * update 3rd-party license file * changes to IScope, ISpan, and IDatadogTracer interfaces * disable current AspNetCoreMvc2 and AspNetCoreMvc3 integrations * add minimal solution * subscribe to DiagnosticSource events from Tracer if enabled * add startup hook * add Datadog.Trace.DiagnosticListeners * add DiagnosticSource configuration * fix URLs to moved repositories * update notes in deprecated NuGet package * fix nuget package comment * remove redundant null coalescing operator * use System.Net.Http from the GAC when targeting net4*, use the nuget package when targeting netstandard2.0 * support ASP.NET Core running on .NET Framework * add logs * remove the build dependency on Datadog.Trace.dll and Datadog.Trace.ClrProfiler.Managed.dll in sample app * fix comment * don't try to write to stderr * remove tailing whitespace * call Tracer.StartDiagnosticObservers from the auto-instrumentation startup hook * stop previous observers when starting new ones in StartDiagnosticObservers() * convert GetListenerName() to property ListenerName * check if there is a parent span earlier to avoid extra work * reorder methods into expected execution order * reference System.Diagnostics.DiagnosticSource 4.4.1, the version used by Microsoft.AspNetCore.* 2.0.0 * move the check for available System.Diagnostics.DiagnosticSource into Tracer * remove remaning sample apps and solution items from the "minimal" solution * remove integration tests for deleted integrations * add missing System.Net.Http references * fix aspnet core mvc integration tests * always save moduel metadata for Microsoft.AspNetCore.Hosting to ensure the startup hook is executed on ASP.NET Core * handle Microsoft.AspNetCore.Diagnostics.UnhandledException event to capture exceptions in ASP.NET Core (not MVC) * use ordinal string compare * stop handling Microsoft.AspNetCore.Mvc.AfterOnActionExecuted since we now have Microsoft.AspNetCore.Diagnostics.UnhandledException * add test app for "pure" ASP.NET Core 3.1 (no MVC) * optimize Log.IsEnabled() calls (only cache the result if it will be used very often) * add checks for error flag and error type in AspNetCoreMvc integration tests * rethrow exceptions in SubmitRequest() so tests fail * add missing netcoreapp3.1 targets in Windows * rename SpanExpectation.Detail() to ToString() and other code clean up * fix WebServerSpanExpectation and AspNetCoreMvcSpanExpectation to properly validate the http status code and http method, and fix the tags for http status code and http method * rewrite SubmitRequest() to use HttpClient instead of WebRequest * stop EntityFramework6x.MdTokenLookupFailure from including Test.Common.props * change wording in sample app * target netcoreapp3.1 in PrepareRelease (resolves nuget restore error) * don't configure an exception handler * add test for 404 * change the default values to unbreak the GraphQL tests

lucaspimentel added type:enhancement Improvement to an existing feature area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. labels Jan 22, 2020

lucaspimentel added this to the 1.12.0 milestone Jan 22, 2020

bobuva reviewed Jan 22, 2020

View reviewed changes

src/Datadog.Trace/DiagnosticListeners/AspNetCoreDiagnostics.cs Outdated Show resolved Hide resolved

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch from 676f2f5 to d5a83e4 Compare January 23, 2020 14:14

lucaspimentel commented Jan 23, 2020

View reviewed changes

src/Datadog.Trace/DiagnosticListeners/PropertyFetcher.cs Outdated Show resolved Hide resolved

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch 3 times, most recently from 5a1c5a7 to f70826c Compare January 27, 2020 17:22

lucaspimentel commented Jan 27, 2020

View reviewed changes

src/Datadog.Trace/DiagnosticListeners/HostingEventProcessor.cs Outdated Show resolved Hide resolved

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch 2 times, most recently from 9fb14c8 to 537dfcc Compare January 28, 2020 03:24

lucaspimentel self-assigned this Jan 28, 2020

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch from 537dfcc to ccfde7a Compare January 28, 2020 23:17

clean up env vars, project properties, and unused references

f2bcb96

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch 2 times, most recently from 4dfccb5 to f97a423 Compare January 29, 2020 17:56

lucaspimentel changed the title ~~[WIP] Use DiagnosticSource to instrument ASP.NET Core~~ Use DiagnosticSource to instrument ASP.NET Core Jan 30, 2020

lucaspimentel removed the status:work-in-progress Actively worked on. If this is a PR, no review needed yet. WIP. label Jan 30, 2020

lucaspimentel marked this pull request as ready for review January 30, 2020 16:12

lucaspimentel requested a review from a team as a code owner January 30, 2020 16:12

lucaspimentel added 8 commits January 30, 2020 11:29

add minimal solution

1b1758c

update 3rd-party license file

efac587

changes to IScope, ISpan, and IDatadogTracer interfaces

457bc3b

disable current AspNetCoreMvc2 and AspNetCoreMvc3 integrations

3fb6362

add Datadog.Trace.DiagnosticListeners

b44f5cf

add DiagnosticSource configuration

cf52e33

subscribe to DiagnosticSource events from Tracer if enabled

18ca210

add startup hook

f64cb5a

lucaspimentel added this to the 1.11.2 milestone Jan 31, 2020

lucaspimentel added 5 commits January 31, 2020 14:05

fix aspnet core mvc integration tests

362ff24

always save moduel metadata for Microsoft.AspNetCore.Hosting to ensur…

810edd6

…e the startup hook is executed on ASP.NET Core

handle Microsoft.AspNetCore.Diagnostics.UnhandledException event to c…

84f026d

…apture exceptions in ASP.NET Core (not MVC)

use ordinal string compare

110c5af

stop handling Microsoft.AspNetCore.Mvc.AfterOnActionExecuted since we…

6e0855a

… now have Microsoft.AspNetCore.Diagnostics.UnhandledException

colin-higgins reviewed Feb 3, 2020

View reviewed changes

lucaspimentel added 8 commits February 3, 2020 14:03

add test app for "pure" ASP.NET Core 3.1 (no MVC)

7374fec

optimize Log.IsEnabled() calls (only cache the result if it will be u…

f4daaa0

…sed very often)

add checks for error flag and error type in AspNetCoreMvc integration…

8143e29

… tests

rethrow exceptions in SubmitRequest() so tests fail

ee2c6c4

add missing netcoreapp3.1 targets in Windows

9066bbf

rename SpanExpectation.Detail() to ToString() and other code clean up

d50cfc4

fix WebServerSpanExpectation and AspNetCoreMvcSpanExpectation to prop…

10ba9a4

…erly validate the http status code and http method, and fix the tags for http status code and http method

rewrite SubmitRequest() to use HttpClient instead of WebRequest

7287ee6

lucaspimentel force-pushed the lpimentel/aspnetcore-diagnosticsource-no-di branch from d3efb89 to 7287ee6 Compare February 4, 2020 14:47

lucaspimentel added 6 commits February 4, 2020 11:09

stop EntityFramework6x.MdTokenLookupFailure from including Test.Commo…

6f9f15f

…n.props

change wording in sample app

17d2e8c

target netcoreapp3.1 in PrepareRelease (resolves nuget restore error)

17d8c09

don't configure an exception handler

ef762d7

add test for 404

c2bc1f2

change the default values to unbreak the GraphQL tests

1234db4

lucaspimentel merged commit e21a59a into master Feb 4, 2020

lucaspimentel deleted the lpimentel/aspnetcore-diagnosticsource-no-di branch February 4, 2020 18:31

lucaspimentel mentioned this pull request Feb 4, 2020

Build Alpine (musl) version of the CLR Profiler #621

Merged

lucaspimentel mentioned this pull request Feb 17, 2020

Getting property values: fix potential cache key conflict, improve speed #642

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use DiagnosticSource to instrument ASP.NET Core #611

Use DiagnosticSource to instrument ASP.NET Core #611

lucaspimentel commented Jan 22, 2020 •

edited

Loading

colin-higgins Feb 3, 2020

lucaspimentel Feb 3, 2020 •

edited

Loading

colin-higgins Feb 3, 2020

lucaspimentel Feb 3, 2020 •

edited

Loading

colin-higgins Feb 3, 2020

lucaspimentel Feb 3, 2020

Use DiagnosticSource to instrument ASP.NET Core #611

Use DiagnosticSource to instrument ASP.NET Core #611

Conversation

lucaspimentel commented Jan 22, 2020 • edited Loading

colin-higgins Feb 3, 2020

Choose a reason for hiding this comment

lucaspimentel Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

colin-higgins Feb 3, 2020

Choose a reason for hiding this comment

lucaspimentel Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

colin-higgins Feb 3, 2020

Choose a reason for hiding this comment

lucaspimentel Feb 3, 2020

Choose a reason for hiding this comment

lucaspimentel commented Jan 22, 2020 •

edited

Loading

lucaspimentel Feb 3, 2020 •

edited

Loading

lucaspimentel Feb 3, 2020 •

edited

Loading