Add NLog instrumentation for OpenTelemetry .NET Auto-Instrumentation#4371
Add NLog instrumentation for OpenTelemetry .NET Auto-Instrumentation#4371Kielek merged 89 commits intoopen-telemetry:mainfrom
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
nrcventura
left a comment
nrcventura
left a comment
There was a problem hiding this comment.
Thank you for submitting this PR. In general, I think these changes look good, but I want to give more time for those that worked on the other bridge implementation to review this PR too.
| [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
Kielek
left a comment
Kielek
left a comment
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
Outdated
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;
| maximumVersion: "6.*.*", | ||
| integrationName: "NLog", | ||
| type: InstrumentationType.Log)] | ||
| public static class WriteLogEventToTargetsIntegration |
There was a problem hiding this comment.
I think that this integration cover only part of the events going through NLog,
There are three WriteLogEventToTargets methods:
private void WriteToTargets(LogLevel level, Exception? ex, IFormatProvider? formatProvider, string message, object?[]? args)- covered, it goes through 2 parameter methosprivate void WriteLogEventToTargets(LogEventInfo logEvent, ITargetWithFilterChain targetsForLevel)- coveredprivate void WriteLogEventToTargets(Type wrapperType, LogEventInfo logEvent, ITargetWithFilterChain targetsForLevel)- not covered
The missing part can be reached by public api: public void Log(Type wrapperType, LogEventInfo logEvent)
EDIT: Please cover this calls in our test application and add appropriate assertions in tests.
...tation/Instrumentations/NLog/TraceContextInjection/Integrations/WriteToTargetsIntegration.cs
Outdated
Show resolved
Hide resolved
.../Instrumentations/NLog/TraceContextInjection/Integrations/WriteToTargetsLegacyIntegration.cs
Outdated
Show resolved
Hide resolved
...tation/Instrumentations/NLog/TraceContextInjection/Integrations/WriteToTargetsIntegration.cs
Outdated
Show resolved
Hide resolved
Kielek
force-pushed
the
feature/nlog-instrumentation
branch
from
187a0ac to
9ba276d
Compare
src/OpenTelemetry.AutoInstrumentation/Instrumentations/NLog/ILoggingEvent.cs
Outdated
Show resolved
Hide resolved
|
|
||
| namespace TestApplication.NLogBridge; | ||
|
|
||
| internal class NLogLogger : Microsoft.Extensions.Logging.ILogger |
There was a problem hiding this comment.
I would consider using this package https://github.com/NLog/NLog.Extensions.Logging
It is common ILogger provider.
log4net implements it in our repository, because similar package was not available at the moment of writing.
Cover Logger.Log(Type wrapperType, LogEventInfo logEvent) API: - WriteLogEventToTargetsWithWrapperTypeIntegration for NLog 6.x - WriteToTargetsWithWrapperTypeIntegration for NLog 5.3.0+ - WriteToTargetsWithWrapperTypeLegacyIntegration for NLog 5.0.0-5.2.x
Replace custom NLogLoggerProvider with official NLog.Extensions.Logging package for ILogger integration testing.
- Use "System.Type" string instead of non-existent ClrNames.Type - Fix trailing newline in ILoggingEvent.cs
Use multiple [InstrumentMethod] attributes per class: - NLogWriteToTargetsIntegration: 2-param methods (3 attributes) - NLogWriteToTargetsWithWrapperTypeIntegration: 3-param methods (3 attributes)
|
Reviewed your changes (and pushed some fixes). The scope of the instrumented methods should be fine, but you missing coverage for them. You need to modify test application and test assertion to cover methods with |
…load - Add LogWithWrapperType method to test application to exercise 3-parameter WriteToTargets/WriteLogEventToTargets instrumentation - Add test expectation for wrapperType log message in NLogBridgeTests - Add NLog.Extensions.Logging to central package management - Update AssertStandardOutputExpectations to verify wrapperType message
…ifitz/opentelemetry-dotnet-instrumentation into feature/nlog-instrumentation
|
Added additional test coverage @Kielek |
Kielek
left a comment
Kielek
left a comment
There was a problem hiding this comment.
I have pushed some changes directly to the PR branch. I think it should be fine to merge.
9 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.