feat(logs): add NLog integration#5176
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5176 +/- ##
==========================================
+ Coverage 74.13% 74.19% +0.06%
==========================================
Files 508 510 +2
Lines 18282 18341 +59
Branches 3574 3583 +9
==========================================
+ Hits 13553 13609 +56
- Misses 3860 3862 +2
- Partials 869 870 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| log.TryGetAttribute("sentry.environment", out object? environment).Should().BeTrue(); | ||
| environment.Should().Be("test-environment"); | ||
| log.TryGetAttribute("sentry.release", out object? release).Should().BeTrue(); | ||
| release.Should().Be("test-release"); | ||
| log.TryGetAttribute("sentry.origin", out object? origin).Should().BeTrue(); | ||
| origin.Should().Be("auto.log.nlog"); | ||
| log.TryGetAttribute("sentry.sdk.name", out object? sdkName).Should().BeTrue(); | ||
| sdkName.Should().Be(Constants.SdkName); | ||
| log.TryGetAttribute("sentry.sdk.version", out object? sdkVersion).Should().BeTrue(); | ||
| sdkVersion.Should().Be(SentryTarget.NameAndVersion.Version); |
There was a problem hiding this comment.
suggestion: reference project Sentry.Testing and simplify assertions with extensions added in #4936
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f558baf. Configure here.
| if (Options.EnableLogs) | ||
| { | ||
| CaptureStructuredLog(hub, Options, logEvent); | ||
| } |
There was a problem hiding this comment.
NLog reads EnableLogs from wrong options source
High Severity
The NLog integration checks Options.EnableLogs (the target's own SentryNLogOptions) and passes Options to CaptureStructuredLog, but the Serilog integration explicitly reads from hub.GetSentryOptions() instead, with a detailed comment explaining why. When the SDK is initialized elsewhere (e.g., ASP.NET Core) and InitializeSdk is false, the NLog target's own Options.EnableLogs remains the default false, so structured logs silently won't be sent even though the user enabled them on the actual SDK options. The options passed to SetDefaultAttributes may also carry incorrect Environment/Release values.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f558baf. Configure here.
There was a problem hiding this comment.
This checks out - I think the same reasoning applies here:
sentry-dotnet/src/Sentry.Serilog/SentrySink.cs
Lines 168 to 175 in d3d2f71
NB: More evidence to support #5245
| return ImmutableArray<KeyValuePair<string, object>>.Empty; | ||
| } | ||
|
|
||
| #if NETSTANDARD2_1_OR_GREATER || NET472_OR_GREATER || NETCOREAPP2_0_OR_GREATER |
There was a problem hiding this comment.
Could potentially simplify this. I think the only scenario where we don't take this codepath is if the TFM is netstandard2.0 right?
There was a problem hiding this comment.
Not a big fan of splitting a single method out into a separate file (and the extra overhead that adds to the csproj file). My preference would be to simply add it to SentryLog.cs... the method name itself lets you know it's a factory method.
jamescrosswell
left a comment
jamescrosswell
left a comment
There was a problem hiding this comment.
Overall looks pretty good. No blocking comments from me - mostly stylistic.
The main one that needs to be dealt with is the one that the Sentry bot called out since that would be a bug for certain initialisation paths.
2 participants
closes #5167
Changes
Add
NLogintegration to Sentry Structured Logging.Docs