Add ASP.NET Core ILogger instrumentation for logs injection by andrewlock · Pull Request #1549 · DataDog/dd-trace-dotnet

andrewlock · 2021-06-17T14:54:47Z

The initial implementation was done by @zacharycmontoya - I added tests and extended to cover .NET Core 3+

This instruments the HostingApplication.ProcessRequestAsync method, which wraps execution of the middleware pipeline, _after the DiagnosticSource.BeginRequest has executed (and hence after a TraceId has been generated).

Limitations:

The integration does not add SpanId to the logs. Doing so would require a different instrumentation point.
The logs injection integration only works for ASP.NET Core requests.
TraceId injection is disabled for .NET Framework. The integration is still enabled, but as the diagnostic-source based tracing doesn't work, the trace_id would always be 0, which defeats the point of logs injection. dd_service, dd_env, and dd_version are added though.

As part of this integration, I also updated the 3.1 sample to run and test against .NET 5, and the 2.1 to test and run against .NET Framework (to confirm existing no-tracing/no-trace_id behaviour)

@DataDog/apm-dotnet

andrewlock · 2021-06-17T14:56:51Z

+namespace Datadog.Trace.AspNetCore
+{
+    // Implementation based on AspNetCore Hosting implementation: https://github.com/dotnet/aspnetcore/blob/1a2b3260c6161ae9b7f639de228a6eb0488a1075/src/Hosting/Hosting/src/Internal/HostingLoggerExtensions.cs#L114
+    internal class DatadogLoggingScope : IReadOnlyList<KeyValuePair<string, object>>


Wonder if we should make this public, so customers can use it with manual instrumentation?

Oh, is this for non-aspnetcore use cases? I was thinking we would start by only exposing an ApplicationBuilder extension method that the user can call, and this could be an implementation detail.

Yeah, I was thinking non-aspnetcore use cases. The ApplicationBuilder makes sense, but then we would have to take dependencies on the aspnetcore packages, which I wasn't sure if we'd want to? We can always make this public later if we want to, so probably makes sense to keep it internal for now.

but then we would have to take dependencies on the aspnetcore packages

In a new Datadog.Trace.AspNetCore package! 😅

andrewlock · 2021-06-17T14:58:14Z

+            {
+                if (index == 0)
+                {
+                    return new KeyValuePair<string, object>("dd_service", _service);


These identifiers are "serilog" style. If users are using ILogger injection and NLog/Log4NET, then they will have two sets of identifiers I think. Not sure what the best way to tackle that is

Yeah they'll have two sets of identifiers...perhaps we'll have a better idea of how to tackle it once we see it in use?

lucaspimentel · 2021-06-24T23:49:58Z

                        "Samples.AspNetCoreMvc21" => Framework == TargetFramework.NETCOREAPP2_1,
                        "Samples.AspNetCoreMvc30" => Framework == TargetFramework.NETCOREAPP3_0,
-                        "Samples.AspNetCoreMvc31" => Framework == TargetFramework.NETCOREAPP3_1,
+                        "Samples.AspNetCoreMvc31" => Framework == TargetFramework.NETCOREAPP3_1 || Framework == TargetFramework.NET5_0,


No Samples.AspNetCoreMvc50? 😝

It's a reasonable question, tbh, and given we have separate projects for 3.0 and 3.1, maybe we should?🤔

Not in this PR of course. To the backlog!

andrewlock · 2021-07-06T12:04:01Z

I wonder if we should provide a way to disable this log injection integration. It's written as part of the aspnetcore integration, but given it isn't totally complete (i.e. it only works for aspnetcore requests, not all ILogger, not manual instrumentation etc), it would add unnecessary duplication if users are also using serilog/nlog/log4net integration.

Those other integrations can be "disabled" by just not wiring them up, but we don't have a way of disabling this ILogger integration, while keeping aspnetcore and Serilog/nlog/log4net log injection enabled. A simple "fix" would be to use a different IntegrationName for this, instead of bundling as part of AspNetCore?

zacharycmontoya · 2021-07-14T21:45:13Z

+        /// <summary>
+        /// Gets the diagnostics field
+        /// </summary>
+        [Duck(Name = "_diagnostics", Kind = DuckKind.Field)]


If #1601 is merged soon, then we can refactor this to

[DuckField(Name = "_diagnostics")]

zacharycmontoya · 2021-07-14T21:47:12Z

I wonder if we should provide a way to disable this log injection integration. It's written as part of the aspnetcore integration, but given it isn't totally complete (i.e. it only works for aspnetcore requests, not all ILogger, not manual instrumentation etc), it would add unnecessary duplication if users are also using serilog/nlog/log4net integration.

Those other integrations can be "disabled" by just not wiring them up, but we don't have a way of disabling this ILogger integration, while keeping aspnetcore and Serilog/nlog/log4net log injection enabled. A simple "fix" would be to use a different IntegrationName for this, instead of bundling as part of AspNetCore?

How about we change the IntegrationName to ILogger, since all of our automatic logs injection for ILogger will have to be done by automatic instrumentation?

zacharycmontoya · 2021-07-14T21:51:06Z

+            where TTarget : IHostingApplication
+        {
+            IDisposable disposable = null;
+            if (Tracer.Instance.Settings.LogsInjectionEnabled)


I think we'll need to modify the check to also check the integration:

if (Tracer.Instance.Settings.LogsInjectionEnabled && Tracer.Instance.Settings.IsIntegrationEnabled(IntegrationId))

paired with

private static readonly IntegrationInfo IntegrationId = IntegrationRegistry.GetIntegrationInfo(IntegrationName);

…onsole logger and file logger that use structured logging

…actual implementation class we'd need to pass to ILogger. The next step is figuring out where to add it to ILogger and how

…automatically add the log scope

- Fix content root - Configure loggers - log using middleware (so we confirm 404s get the trace id etc too) - Add Startup log that will not be traced - Exclude other logs by default

Could/Should make these separate tests, but keeping them the same for now for speed/simplicity

Avoids unexpected TraceId: 0

Don't support tracing yet, so won't get any TraceIds, but can inject service/env/version into logs

This wasn't possible before, because the .NET Core 2.1 implementation of ILogger, Logger, used explicit interface implementation. This is now supported out of the box thanks to #1555

Co-authored-by: Lucas Pimentel-Ordyna <lucas.pimentel@datadoghq.com>

zacharycmontoya

LGTM, and since I hadn't looked at this in a while I think I acquired an objective perspective to review it properly 😆

andrewlock · 2021-08-23T14:51:26Z

Superseded by #1663. I'll pull the additional tests (for .NET Framework etc) out in a separate PR

andrewlock requested a review from a team as a code owner June 17, 2021 14:54

andrewlock commented Jun 17, 2021

View reviewed changes

Comment thread src/Datadog.Trace.ClrProfiler.Managed/AutoInstrumentation/AspNetCore/ILoggerIntegration.cs Outdated

andrewlock commented Jun 17, 2021

View reviewed changes

lucaspimentel added area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) type:new-feature labels Jun 17, 2021

andrewlock force-pushed the andrew/feature/ilogger-injection branch 5 times, most recently from 2c1597d to bf5b530 Compare June 22, 2021 09:46

andrewlock mentioned this pull request Jun 23, 2021

Add support for NestedTypes and GenericParameters to EnsureTypeVisibility #1561

Merged

lucaspimentel reviewed Jun 24, 2021

View reviewed changes

Comment thread src/Datadog.Trace/DuckTyping/DuckTypeExceptions.cs Outdated

lucaspimentel approved these changes Jun 24, 2021

View reviewed changes

andrewlock force-pushed the andrew/feature/ilogger-injection branch 3 times, most recently from 1dde0da to eca92e8 Compare June 30, 2021 10:02

andrewlock force-pushed the andrew/feature/ilogger-injection branch 2 times, most recently from 4e1fd51 to 98d0e8c Compare July 13, 2021 08:11

zacharycmontoya reviewed Jul 14, 2021

View reviewed changes

zacharycmontoya and others added 6 commits July 15, 2021 15:07

Prepare netcoreapp2.1, netcoreapp3.0, and netcoreapp3.1 apps with a c…

4a3287e

…onsole logger and file logger that use structured logging

Modify program to write to Serilog and to use the first draft of the …

0c843c4

…actual implementation class we'd need to pass to ILogger. The next step is figuring out where to add it to ILogger and how

Update sample ASP.NET Core 2.1 application to not add logging manually

cbb53f2

Add automatic instrumentation that gets enabled on AspNetCore 2.1 to …

88798e6

…automatically add the log scope

Allow providing a specific tracer to DatadogLoggingScope (for testing)

1ed1f30

Add unit tests for DatadogLoggingScope based on LogProvider tests

1ed6249

andrewlock and others added 16 commits July 15, 2021 15:07

Fix incorrect use of local time in ASP.NET Core tests

0fdb488

Update AspNetCore log testing implementation

7a1e03b

- Fix content root - Configure loggers - log using middleware (so we confirm 404s get the trace id etc too) - Add Startup log that will not be traced - Exclude other logs by default

Verify log injection as part of ASP.NET Core tests

e196e26

Could/Should make these separate tests, but keeping them the same for now for speed/simplicity

Update ILoggerIntegration to support .NET Core 3.x and 5.x

e93faf2

Update 3.0 and 3.1 apps with custom logger work

d7bd3e7

Update AspNetCoreMvc tests to test log injection

04c79a0

Refactor ASP.NET Core ILogger injection tests

986a401

Add ASP.NET Core tests for .NET 5

b5a3552

Update DatadogLoggingScope to only inject TraceId if a trace is active

68df604

Avoids unexpected TraceId: 0

Run tests for ASP.NET Core on NET Framework

b05d52c

Don't support tracing yet, so won't get any TraceIds, but can inject service/env/version into logs

Cleanup samples

05c0964

Simplify DuckTyping implementation

5fe2368

This wasn't possible before, because the .NET Core 2.1 implementation of ILogger, Logger, used explicit interface implementation. This is now supported out of the box thanks to #1555

Update src/Datadog.Trace/DuckTyping/DuckTypeExceptions.cs

7ad264b

Co-authored-by: Lucas Pimentel-Ordyna <lucas.pimentel@datadoghq.com>

Remove unused duck type helpers

110000f

Use a separate integration name for ILogger

d773a13

Update integrations.json

9ff60d4

andrewlock force-pushed the andrew/feature/ilogger-injection branch from 98d0e8c to 9ff60d4 Compare July 15, 2021 14:23

zacharycmontoya approved these changes Jul 15, 2021

View reviewed changes

andrewlock mentioned this pull request Aug 5, 2021

Add Microsoft.Extensions.Logging.ILogger instrumentation #1663

Merged

andrewlock closed this Aug 23, 2021

andrewlock deleted the andrew/feature/ilogger-injection branch August 23, 2021 14:51

andrewlock restored the andrew/feature/ilogger-injection branch August 23, 2021 14:51

andrewlock mentioned this pull request Aug 31, 2021

Add tests for ASP.NET Core on .NET 5.0 and .NET Framework 4.6.1 #1716

Closed

Conversation

andrewlock commented Jun 17, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewlock commented Jul 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya commented Jul 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants