Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET 5.0 removes SpanId and ActivityId #445

Closed
snakefoot opened this issue Aug 9, 2020 · 11 comments · Fixed by #456
Closed

.NET 5.0 removes SpanId and ActivityId #445

snakefoot opened this issue Aug 9, 2020 · 11 comments · Fixed by #456
Labels
Milestone

Comments

@snakefoot
Copy link
Contributor

ActivityId has been removed as default outer-scope-property. Instead it is injected using LoggerExternalScopeProvider:

dotnet/runtime#34305
dotnet/runtime#37092

Because NLog doesn't make use of LoggerExternalScopeProvider, then NLog might have to implements its own logic to check ActivityTrackingOptions. Alternative just rely on the layout-renderer from NLog.DiagnosticSource.

Also notice that .NET 5 adds implicit dependency from Microsoft.Extensions.Logging to System.Diagnostics.DiagnosticSource:

https://www.nuget.org/packages/Microsoft.Extensions.Logging

So one could merge NLog.Extensions.Logging together with NLog.DiagnosticSource

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 2, 2020

Guess if one waits long enough, then the problems become irrelevant by themselves: dotnet/runtime#34305

public static IHostBuilder CreateHostBuilder(string[] args) =>
    Host.CreateDefaultBuilder(args)
        .ConfigureLogging(loggingBuilder =>
            loggingBuilder.Configure(options =>
                options.ActivityTrackingOptions =
                    ActivityTrackingOptions.TraceId
                    | ActivityTrackingOptions.SpanId))
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.UseStartup<Startup>();
        });

@Oskarsson
Copy link

Oskarsson commented Oct 2, 2020

@snakefoot This should still be open? Nlog.Extensions.Logging is not using LoggerFactory as the base class and because of this the ActivityTrackingOptions option not supported.

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 2, 2020 via email

@Oskarsson
Copy link

Oskarsson commented Oct 2, 2020

hmm, then there is something else because the default options for the ActivityTrackingOptions is already ActivityTrackingOptions.TraceId | ActivityTrackingOptions.SpanId | ActivityTrackingOptions.ParentId and still neither TraceId, SpanId or ParentId is shown as a scopes with the Nlog provider and it works perfectly fine with the ConsoleProvider. Just tried with a fresh solution running .NET 5 RC 1 and latest Nlog.Extensions.Logging

@snakefoot snakefoot reopened this Oct 2, 2020
@snakefoot
Copy link
Contributor Author

Think I need to put on my thinking hat again. Yes this issue is still relevant. Could probably add a new NLogProviderOptions-option to NLog NetCoreApp3-framework-build.

When .Net5 arrives then one can consider to enable the option by default, unless performance is more important.

@snakefoot
Copy link
Contributor Author

snakefoot commented Oct 2, 2020

The challenge with capturing ActivityId + SpanId dynamically is that they reside in their own static AsyncLocal-variable.

You don't want to clone/push them to the NLog-MappedContext/ScopeContext, because it would be a stale copy. But you want to perfom the capture together with the logevent to handle async-logging.

The standard solution would be adding ActivityId + SpanId using dedicated Layout, but then you have to manual add them to every target/layout. See NLog.DiagnosticSource.

If wanting some automatic registration/inclusion of fixed ScopeContext-properties, then one could consider extending LoggingConfiguration to register these fixed-properties (Name + Layout) for MappedContext/ScopeContext-properties. Then have TargetWithContext/JsonLayout/XmlLayout etc to ask the LogFactory when needing to query MappedContext/ScopeContext-properties, and it could do the automatic injecting on the actual logevent. This mean any legacy class that query the NLog MDLC directly will not get the fixed-properties.

Guess this feature sounds a lot like Serilog Enrichers, that ensures properties are injected automatically for all LogEvents. NLog has seveeral types of properties:

  • LogEvent Properties - One can tell the NLog Logger (Not Microsoft ILogger) to include default properties using WithProperty.
  • Scope Properties - MDLC.
  • Layout Properties - Ex JsonLayout Attributes
  • Target Properties - Ex TargetWithContext Properties

One could consider if it should be possible to configure Logger-default-properties and Scope-default-properties in the LoggingConfiguration also ? (Right now only possible for Layout-properties and Target-properties).

There is a challenge in making it easy to merge NLog.config with runtime-adjustments of Nlog.config, and also handle autoReload. Just like NLog Config Variables has hidden surprises. Probably need some kind of "ownership" so runtime-adjustments becomes sacred and will not be overwritten by reload / autoreload.

@snakefoot
Copy link
Contributor Author

snakefoot commented Nov 23, 2020

@Oskarsson Would it make sense to add a new NLogProviderOptions-option called IncludeActivtyIdsWithBeginScope.

So when AspNetCore performs BeginScope with "RequestId" and "RequestPath" then NLog will automatically inject the following properties:

  • System.Diagnostics.Activity.SpanId
  • System.Diagnostics.Activity.TraceId
  • System.Diagnostics.Activity.ParentId

When NLog 5.0 is release then it will introduce a new ScopeContext-feature, where it is possible to register default scope-properties where the value comes from NLog Layouts. Thus allowing one to setup default scope-properties like SpanId, TraceId, ParentId (or other values from NLog layout-renderers) independent of them being provided by BeginScope.

So the option IncludeActivtyIdsWithBeginScope will become obsolete again when NLog 5.0 is released, and instead replaced with a LogFactory.Setup()-extension-method. Maybe with ActivityTrackingOptions as input-parameter to the extension-method.

@snakefoot
Copy link
Contributor Author

Created #456 as an example of this idea.

@snakefoot
Copy link
Contributor Author

PreRelease nuget-packages are available, so one can test the new option IncludeActivtyIdsWithBeginScope with .Net5.0

@304NotModified
Copy link
Member

Fixed by #456

@snakefoot snakefoot added this to the 1.7 milestone Jan 7, 2021
@snakefoot
Copy link
Contributor Author

snakefoot commented Jan 9, 2021

Alternative to IncludeActivtyIdsWithBeginScope is using these with <contextproperty>:

  • ${activity:SpanId}
  • ${activity:ParentId}
  • ${activity:TraceId}
  • And many more (docs)

Nuget-package NLog.DiagnosticSource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants