Skip to content

feat: NLog target no longer initializes the SDK - #5585

Open
jamescrosswell wants to merge 14 commits into
feat/no-init-from-logging-5245from
feat/no-init-from-logging-nlog-5245
Open

jamescrosswell wants to merge 14 commits into
feat/no-init-from-logging-5245from
feat/no-init-from-logging-nlog-5245

Conversation

@jamescrosswell

@jamescrosswell jamescrosswell commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

The NLog portion of #5245, stacked on #5573 (Serilog) and following the same design. The Sentry target for NLog now only configures the target; Sentry has to be initialized separately via SentrySdk.Init, UseSentry, etc.

Part of #5245

Breaking changes

  • SentryNLogOptions no longer derives from SentryOptions. It carries only target settings.
  • Removed from SentryNLogOptions and SentryTarget: InitializeSdk, Dsn/DsnLayout, Release/ReleaseLayout, Environment/EnvironmentLayout, ShutdownTimeoutSeconds, FlushTimeout/FlushTimeoutSeconds. Events now take release and environment from the options used to initialize Sentry, rather than per-target overrides.
  • When NLog flushes the target, Sentry is flushed using SentryOptions.FlushTimeout from the options used to initialize Sentry. That defaults to 2 seconds; the NLog target previously defaulted to 15. Set FlushTimeout in SentrySdk.Init to keep the old wait.
  • In NLog.config, the dsn, release, environment, initializeSdk, shutdownTimeoutSeconds and flushTimeoutSeconds target attributes are gone, as is setting arbitrary SentryOptions properties through the <options> element (e.g. <options attachStacktrace="true" />). With throwConfigExceptions="true" these now fail config loading.
  • dsn and initializeSdk specifically fail with migration guidance rather than "cannot assign unknown property" — see the tombstones below.
  • The three AddSentry overloads become one supported overload, AddSentry(Action<SentryNLogOptions>? optionsConfig = null, string targetName = "sentry"), plus two tombstones.
  • Events and structured logs no longer report sentry.dotnet.nlog as the SDK name. Sdk.Name identifies the integration that initialized Sentry, and the target identifies itself through the log origin (auto.log.nlog). See Metrics and SentrySdk.Logger logs emitted during a request carry no sentry.sdk.name/sentry.sdk.version on ASP.NET Core #5497.
  • SDK diagnostics are no longer written to NLog's InternalLogger. Previously, enabling NLog internal logging at any level made the target set the SDK's DiagnosticLogger to write there and turn on Debug. To see SDK diagnostics, set Debug (and optionally DiagnosticLogger) on the options used to initialize Sentry.

Before:

LogManager.Configuration = new LoggingConfiguration()
    .AddSentry("https://key@sentry.io/1", o => o.MinimumEventLevel = LogLevel.Error);

After:

using var _ = SentrySdk.Init(o => o.Dsn = "https://key@sentry.io/1");

LogManager.Configuration = new LoggingConfiguration()
    .AddSentry(o => o.MinimumEventLevel = LogLevel.Error);

Migration guard (tombstones)

Following the pattern established for Serilog in #5611, the v6 entry points that initialized the SDK are kept as tombstones rather than deleted: AddSentry(dsn, …), AddSentry(dsn, targetName, …), SentryTarget.Dsn and SentryTarget.InitializeSdk. Each is [Obsolete(…, error: true)] and throws NotSupportedException with migration guidance.

ObsoleteAttribute has no runtime effect, so NLog still finds these by reflection when binding NLog.config, and the setter throws — which NLog surfaces as an NLogConfigurationException carrying our message. Code callers get a compile error instead of the silent behaviour change.

Without them, a stale dsn="…" in NLog.config produces only NLog's generic "cannot assign unknown property", and with throwConfigExceptions unset that is a warning to InternalLogger — off by default. The app starts, the target attaches, Sentry is never initialized and nothing is reported.

Covered by SentryTargetConfigurationBindingTests: XML config carrying dsn or initializeSdk fails with our message, target-only settings still load, and both overloads throw when invoked by name (as a config provider does) and are obsolete-as-error.

Notes for review

  • targetName moved to the last parameter on purpose. Keeping a (string targetName, Action<SentryNLogOptions>) overload would let existing AddSentry(dsn, o => …) calls keep compiling, with the DSN silently used as the target name and Sentry never initialized. With targetName last, every old DSN-taking call fails to compile instead.
  • Why the flush timeout moved to the SDK options. NLog flushes its targets on LogManager.Flush(), on LogManager.Shutdown() (which NLog also calls itself on process exit, since AutoShutdown is on by default), when the configuration is replaced (including autoReload), and from wrappers such as AutoFlushTargetWrapper. NLog doesn't pass its own timeout down to targets, so the target has to pick one, and the SDK-wide flush it triggers covers events from every integration, not only NLog. Previously the target owned the SDK, so this flush on shutdown was how an NLog-only app got its last events sent. Now the app initializes and disposes Sentry itself, and disposing the handle from SentrySdk.Init flushes on its own. The NLog-triggered flush is still useful for explicit flushes, config reloads and auto-flush wrappers, but it's an SDK operation, so it uses the SDK's setting instead of a target-level duplicate.
  • Why NLogDiagnosticLogger is deleted. Routing SDK diagnostics to InternalLogger was SDK configuration applied at target initialization. The target's options are no longer SentryOptions, and applying it to the live hub options instead would mean the target silently reconfiguring (and switching on Debug for) an SDK the user initialized. With no remaining callers, the internal class was dead code.
  • Unlike Serilog, NLog needs no UseNLog(): tags, user and properties are all applied by the target to the events it creates.
  • The IntegrationTests.Simple snapshot changes are only a stack-frame line/column shift from restructuring the test. Net4_8 got the same edit by hand, and ApiApprovalTests.Run.Net4_8 is a copy of the regenerated DotNet10_0 one, which it matched byte-for-byte beforehand. Neither can regenerate on macOS.
  • The integration tests previously never disposed the SDK the target initialized; they now dispose it via using before verifying.

🤖 Generated with Claude Code

The Sentry target for NLog now only configures the target. Sentry must be
initialized separately (SentrySdk.Init, UseSentry, etc).

- SentryNLogOptions no longer derives from SentryOptions and only carries
  target settings; FlushTimeout moves onto it directly
- Remove InitializeSdk, Dsn/DsnLayout, Release/ReleaseLayout,
  Environment/EnvironmentLayout and ShutdownTimeoutSeconds. Events take
  release and environment from the SDK options
- Collapse the AddSentry overloads into
  AddSentry(optionsConfig, targetName); the dsn overloads are removed
- The target no longer routes SDK diagnostics to NLog's InternalLogger

Part of #5245

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jamescrosswell jamescrosswell added Breaking Change Binary/Source/Behavioral Breaking Changes. NLog labels Sep 17, 2026
@codecov

codecov Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.64%. Comparing base (bbf2ffd) to head (a8a1ea9).

Files with missing lines Patch % Lines
src/Sentry.NLog/SentryTarget.cs 62.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                        Coverage Diff                         @@
##           feat/no-init-from-logging-5245    #5585      +/-   ##
==================================================================
- Coverage                           74.69%   74.64%   -0.06%     
==================================================================
  Files                                 515      514       -1     
  Lines                               18845    18760      -85     
  Branches                             3668     3647      -21     
==================================================================
- Hits                                14077    14003      -74     
+ Misses                               3894     3889       -5     
+ Partials                              874      868       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread samples/Sentry.Samples.NLog/Program.cs Outdated
Comment thread samples/Sentry.Samples.NLog/README.md Outdated
Comment thread samples/Sentry.Samples.NLog/README.md Outdated
Comment thread src/Sentry.NLog/SentryNLogOptions.cs Outdated
jamescrosswell and others added 2 commits September 17, 2026 15:29
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
Remove SentryTarget.FlushTimeoutSeconds and SentryNLogOptions.FlushTimeout.
When NLog flushes the target, the hub is now flushed with the FlushTimeout
from the options used to initialize Sentry, since the target no longer
owns the SDK.

Part of #5245

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jamescrosswell and others added 10 commits September 22, 2026 17:52
Sdk.Name should identify the integration that initialised the hub, which after
this change can no longer be a logging integration. The target identifies itself
through the log origin (auto.log.nlog) instead.
See #5497.

Events are no longer stamped with sentry.dotnet.nlog, and structured logs no
longer carry it as sentry.sdk.name; both now report the SDK that initialised
Sentry. With no remaining callers, Constants is deleted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n error

Mirrors the Serilog guard (#5611). The v6 AddSentry(dsn, ...) overloads and
the SentryTarget.Dsn / InitializeSdk properties come back as tombstones:
obsolete-as-error for code callers, throwing NotSupportedException so
NLog.config bindings fail loudly with migration guidance instead of
reporting an unknown property.

Part of #5245

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking Change Binary/Source/Behavioral Breaking Changes. NLog risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant