Add NLog instrumentation for OpenTelemetry .NET Auto-Instrumentation #4371
Conversation
This commit implements a comprehensive NLog instrumentation that automatically bridges NLog logging events to OpenTelemetry without requiring code changes. Features: - Automatic target injection into NLog's target collection - Complete log event bridging with level mapping - Structured logging support with message templates - Trace context integration - Custom properties forwarding with filtering - Comprehensive test coverage - End-to-end test application - Extensive documentation The implementation follows the same patterns as the existing Log4Net instrumentation and supports NLog versions 4.0.0 through 6.*.*. Configuration: - OTEL_DOTNET_AUTO_LOGS_ENABLED=true - OTEL_DOTNET_AUTO_LOGS_ENABLE_NLOG_BRIDGE=true - OTEL_DOTNET_AUTO_LOGS_INCLUDE_FORMATTED_MESSAGE=true (optional) Files added: - src/OpenTelemetry.AutoInstrumentation/Instrumentations/NLog/ - test/OpenTelemetry.AutoInstrumentation.Tests/NLogTests.cs - test/test-applications/integrations/TestApplication.NLog/ Files modified: - Configuration classes to support NLog bridge - Public API files to include new integration class
|
|
|
I will update the changelog and documentation once we've verified that the code itself looks okay |
test/test-applications/integrations/TestApplication.NLog/TestApplication.NLog.csproj
Outdated
Show resolved
Hide resolved
| [InstrumentMethod( | ||
| assemblyName: "NLog", | ||
| typeName: "NLog.Config.LoggingConfiguration", | ||
| methodName: "GetConfiguredNamedTargets", |
There was a problem hiding this comment.
I can't find method with this name in any recent version of NLog.
There was a problem hiding this comment.
Sorry I made an incorrect assumption when following some of the work in Log4Net bridge instrumentation. I had to make quite a few changes in #1b5ee77
...erators/SourceGenerators.InstrumentationDefinitionsGenerator/InstrumentationDefinitions.g.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
We usually use test apps in integration tests, as in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/186e653ad1a38e5c0b876f71aeef03bf3630cf7f/test/IntegrationTests/Log4NetBridgeTests.cs.
If we were to use this app for that purpose, it should probably be reworked.
Project should also be added to the solution.
There was a problem hiding this comment.
I reviewed the approach and made some changes in 638a6aa
| .ConfigureLogging(logging => | ||
| { | ||
| // Clear default logging providers | ||
| logging.ClearProviders(); |
There was a problem hiding this comment.
This clears all providers, including the one injected by autoinstrumentation.
There was a problem hiding this comment.
this should be fixed by the test refactor I did in 638a6aa
| /// The target integrates with NLog's architecture by implementing the target pattern, | ||
| /// allowing it to receive log events and forward them to OpenTelemetry for processing. | ||
| /// </summary> | ||
| internal class OpenTelemetryNLogTarget |
There was a problem hiding this comment.
Maybe choose a different class-name, since NLog Target is a reserved word. Maybe OpenTelemetryNLogOutput or OpenTelemetryNLogMapper or OpenTelemetryNLogConverter or OpenTelemetryNLogDestination.
Why not implement a standard NLog Target instead of all this reflection ?
NLog Logger -> NLog OpenTelemetryTarget -> OpenTelemetry Output.
There was a problem hiding this comment.
Here is a very nice example of a NLog target for OpenTelemetryTarget using TargetWithContext, that allows one to enrich LogEvents with additional properties, and also configure from NLog.config-file:
See also: https://github.com/NLog/NLog/wiki/How-to-write-a-custom-target-for-structured-logging
There was a problem hiding this comment.
Hi @snakefoot thanks for your comments.
Given that this is for auto-instrumentation (zero-config), I feel like the current approach makes sense from that perspective. However, your points are spot on.
What do you think? Should we:
- Keep the current auto-instrumentation approach but fix the naming?
- Switch to a standard NLog Target approach (no longer zero-config)?
- Implement both approaches - auto-instrumentation for zero-config, plus a Target for manual configuration?
Your insight into using standard NLog patterns is helpful - it would result in cleaner code that follows NLog best practices.
There was a problem hiding this comment.
NLog encourages the ability to configure the NLog Target from code or from NLog.config-file, instead of auto-instrumentation. Ofcourse good default values are encourages so the NLog Targets works out-of-the-box with no or little configuration.
When using TargetWithContext then one can use the NLog Layout abilities to enrich the OpenTelemetry-Logging-output with additional logeevent-properties. The NLog LoggingRules provides a lot of flexibility in filtering / routing NLog Logging output to the wanted target-destinations, but requires NLog targets.
There was a problem hiding this comment.
I'm taking a week off to go on holiday - i wil look to refactor this when I get back, thanks
There was a problem hiding this comment.
Hi folks, I just now took a look at this PR and added a comment to request that we in fact go back to zero-code, so the original implementation of the OpenTelemetryNLogTarget looks like exactly what we want.
I apologize that I didn't comment sooner that zero-code is the direction we should take with this work. The reason for favoring this approach is that this project should not contain any other library dependencies except for the OTel SDK, so we should not be creating derived types that extend types from the NLog library.
…ogger.Log method The original integration targeted a non-existent method 'GetConfiguredNamedTargets' in recent NLog versions. Changed to intercept the actual NLog.Logger.Log(LogEventInfo) method which is stable across NLog 4.0+ versions. - Renamed TargetCollectionIntegration to LoggerIntegration - Changed from OnMethodEnd to OnMethodBegin approach - Updated public API references - Fixes instrumentation for all NLog versions 4.0-6.*.*
…ition for net462
…s following Log4NetBridge pattern Rework TestApplication.NLog into TestApplication.NLogBridge with proper integration test support. Add NLogBridgeTests.cs with complete coverage of direct NLog usage, ILogger bridge, and trace context injection. - Add to solution and LibraryVersionsGenerator - Fix config to remove Windows-specific paths - Support --api nlog and --api ILogger test modes
There was a problem hiding this comment.
@danifitz, @snakefoot - I like the idea to introduce support for NLog here. If some/all cases cane be done by NLog directly, it is great (maybe context correlation can be done, without any changes here and we only need to document it/enable by default?)
The goal of the support should be also possibility to automatically also sent data through OTLP without any changes in the configuration/code.
...Telemetry.AutoInstrumentation/Instrumentations/NLog/Bridge/Integrations/LoggerIntegration.cs
Show resolved
Hide resolved
test/test-applications/integrations/TestApplication.NLogBridge/TestApplication.NLog.csproj
Outdated
Show resolved
Hide resolved
| <PackageReference Include="NLog.Extensions.Logging" VersionOverride="$(LibraryVersion)" /> | ||
| <PackageReference Include="Microsoft.Extensions.Logging"/> | ||
| <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" /> | ||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" /> |
There was a problem hiding this comment.
I am not fully familiar with NLog details. Is it necessary to bring all these dependencies to make it working? Can this list be reduced?
…tecture - Replace CallTarget interception with standard NLog.Targets.TargetWithContext - Add OpenTelemetry.AutoInstrumentation.NLogTarget project with OpenTelemetryTarget - Implement NLogAutoInjector for zero-config auto-injection via CallTarget - Rename OpenTelemetryNLogTarget to OpenTelemetryNLogConverter for clarity - Update LoggerIntegration to trigger auto-injection and set GlobalDiagnosticsContext - Rework TestApplication.NLogBridge to follow standard integration test pattern - Update NLogBridgeTests to match Log4NetBridgeTests structure - Add InternalsVisibleTo for new NLog target project access - Update documentation to reflect dual-path architecture (auto-injection + manual config) - Remove obsolete files and clean up project structure This refactor aligns with NLog best practices by providing both automatic instrumentation and a standard NLog Target that can be configured via nlog.config or programmatically, leveraging NLog's native layout and routing capabilities.
…NLogBridge The package was not referenced in any source files and the test app implements its own ILogger bridge for testing purposes.
|
I took onboard the comments from @snakefoot and @Kielek and undertook a refactor to align more with the NLog way of doing things. The key aspects of this refactor:
206be87 represents an improvement in the NLog instrumentation's architecture, making it more maintainable and aligned with NLog's intended usage patterns. |
| } | ||
|
|
||
| [RequiredParameter] | ||
| public string? Endpoint { get; set; } |
| [RequiredParameter] | ||
| public string? Endpoint { get; set; } | ||
|
|
||
| public string? Headers { get; set; } |
|
|
||
| public bool UseHttp { get; set; } = true; | ||
|
|
||
| public string? ServiceName { get; set; } |
| var factory = _getLoggerFactory; | ||
| if (factory is null) | ||
| { | ||
| return new object(); |
There was a problem hiding this comment.
This looks weird. Why not allow object? and just discards LogEvents when GetOrCreateLogger returns null ?
| var current = Activity.Current; | ||
|
|
||
| // Emit using internal helpers via reflection delegate | ||
| var renderedMessage = logEvent.FormattedMessage; |
There was a problem hiding this comment.
Instead change to:
var renderedMessage = RenderLogEvent(Layout, logEvent);
|
|
||
| // Emit using internal helpers via reflection delegate | ||
| var renderedMessage = logEvent.FormattedMessage; | ||
| var args = IncludeEventParameters && logEvent.Parameters is object[] p ? p : null; |
There was a problem hiding this comment.
Think you should only IncludeEventParameters when logEvent.HasProperties == false
|
|
||
| // Build properties from event properties and context | ||
| var properties = new List<KeyValuePair<string, object?>>(); | ||
| if (IncludeEventProperties && logEvent.HasProperties && logEvent.Properties is not null) |
| // Scope properties can be added via explicit <attribute> entries or NLog's contexts (GDC/MDLC) | ||
| foreach (var attribute in Attributes) | ||
| { | ||
| var value = attribute.Layout?.Render(logEvent); |
There was a problem hiding this comment.
instead use the following, or just remove Attributes and instead use GetAllProperties(logEvent).
var value = RenderLogEvent(attribute.Layout, logEvent);
if (!attribute.IncludeEmptyValue && string.IsNullOrEmpty(value))
continue;
| } | ||
| } | ||
|
|
||
| var body = IncludeFormattedMessage ? logEvent.FormattedMessage : Convert.ToString(logEvent.Message); |
There was a problem hiding this comment.
Instead change to:
var renderedMessage = IncludeFormattedMessage ? RenderLogEvent(Layout, logEvent) : logEvent.Message;
it is not used
There is no tests for these versions
|
Thanks for cleaning this up @Kielek - any further action needed from me? |
seems to be redundant
|
@danifitz, still reviewing. In general looks good, for applied small changes, it was easier to make commits directly than writing a comments. I hope you are fine with this. |
I'm so fine with that :) |
There was a problem hiding this comment.
Hi, I have spend some time trying to fix.
The infrastructure + documentation now looks good to me. The problematic part is that basically is not working to me, any of the implemented tests nor manual scenarios.
Please let me know if the comments are meaningful, if no I will try to provide more information.
| NLogAutoInjector.EnsureConfigured(); | ||
|
|
||
| // Inject trace context into NLog GlobalDiagnosticsContext for current destination outputs | ||
| TrySetTraceContext(Activity.Current); |
There was a problem hiding this comment.
I think that this method should be called always, even if the bridge is not enabled.
Bridge means that we are sending NLog logs through OTel infrastructure. Typically OTLP.
You need to set context also when you are injecting in to the typical NLog sinks.
| [InstrumentMethod( | ||
| assemblyName: "NLog", | ||
| typeName: "NLog.Logger", | ||
| methodName: "Log", | ||
| returnTypeName: ClrNames.Void, | ||
| parameterTypeNames: new[] { "NLog.LogEventInfo" }, |
There was a problem hiding this comment.
I think that you need to revisit the way how you injecting bytecode.
Logger.log(LogEventInfo) is not covering all cases when you are trying to log.
Even when I have checked the testing application calling this method, it is failing.
| /// <summary> | ||
| /// Converts NLog LogEventInfo into OpenTelemetry LogRecords. | ||
| /// </summary> | ||
| internal class OpenTelemetryNLogConverter |
There was a problem hiding this comment.
I do not found any usage of this class except the tests.
|
I'm a bit busy over the next few days but will try and find some time to look into these comments next week or the weekend |
|
Still planning to resolve this but have been busy recently |
7 participants
This commit implements a comprehensive NLog instrumentation that automatically bridges NLog logging events to OpenTelemetry without requiring code changes.
Features:
The implementation follows the same patterns as the existing Log4Net instrumentation and supports NLog versions 4.0.0 through 6...
Configuration:
Files added:
Files modified:
Why
#3938
Fixes #3938
What
This PR implements a comprehensive NLog instrumentation that automatically bridges NLog logging events to OpenTelemetry without requiring any code changes from users.
Tests
Checklist
CHANGELOG.mdis updated.