Repository navigation
Open
Labels
.NETPull requests that update .net codePull requests that update .net codeFeatureNew feature or requestNew feature or requestLogs
Description
This is the current code:
sentry-dotnet/src/Sentry/SentryOptions.cs
Lines 620 to 668 in 6274944
| /// <summary> | |
| /// Whether to log diagnostics messages | |
| /// </summary> | |
| /// <remarks> | |
| /// The verbosity can be controlled through <see cref="DiagnosticLevel"/> | |
| /// and the implementation via <see cref="DiagnosticLogger"/>. | |
| /// </remarks> | |
| public bool Debug | |
| { | |
| get => _debug; | |
| set => _debug = value; | |
| } | |
| /// <summary> | |
| /// The diagnostics level to be used | |
| /// </summary> | |
| /// <remarks> | |
| /// The <see cref="Debug"/> flag has to be switched on for this setting to take effect. | |
| /// </remarks> | |
| public SentryLevel DiagnosticLevel { get; set; } = SentryLevel.Debug; | |
| private volatile IDiagnosticLogger? _diagnosticLogger; | |
| /// <summary> | |
| /// The implementation of the logger. | |
| /// </summary> | |
| /// <remarks> | |
| /// The <see cref="Debug"/> flag has to be switched on for this logger to be used at all. | |
| /// When debugging is turned off, this property is made null and any internal logging results in a no-op. | |
| /// </remarks> | |
| public IDiagnosticLogger? DiagnosticLogger | |
| { | |
| get => Debug ? _diagnosticLogger : null; | |
| set | |
| { | |
| if (value is null) | |
| { | |
| _diagnosticLogger?.LogDebug( | |
| "Sentry will not emit SDK debug messages because debug mode has been turned off."); | |
| } | |
| else | |
| { | |
| _diagnosticLogger?.LogInfo("Replacing current logger with: '{0}'.", value.GetType().Name); | |
| } | |
| _diagnosticLogger = value; | |
| } | |
| } |
What this effectively does, AFAICT, is that we don't print any warnings when Debug is not set. That is pretty unexpected behavior, at least to me, especially considering what the docs say when you're setting up sentry:
// When debug is enabled, the Sentry client will emit detailed debugging information to the console.
// This might be helpful, or might interfere with the normal operation of your application.
// We enable it here for demonstration purposes when first trying Sentry.
// You shouldn't do this in your applications unless you're troubleshooting issues with Sentry.
options.Debug = true;
Instead, I'd expect the DiagnosticLogger property to always be reachable. It already has a check which log levels should be printed.
Metadata
Metadata
Assignees
Labels
.NETPull requests that update .net codePull requests that update .net codeFeatureNew feature or requestNew feature or requestLogs
Projects
- StatusShow more project fieldsDone
Activity
bitsandfoxes commentedon Mar 14, 2024
Looks like
Debugcontrols whether the SDK adds its ownConsoleLoggerasDiagnosticLoggerif none is present.sentry-dotnet/src/Sentry/SentryOptions.cs
Lines 1636 to 1645 in dfe208f
You can set your own logger and that works independently whether
Debugis enabled or not.vaind commentedon Mar 14, 2024
Yes you can. Yet again, shouldn't the default at least print warnings and errors, if any occur? As is, it won't print anything unless you set
Debugin which case it will print not only warnings but also all the debug messages.vaind commentedon Mar 14, 2024
Actually, no, you cannot set a diagnostic logger.
sentry-dotnet/src/Sentry/SentryOptions.cs
Lines 651 to 653 in 6274944
Can we just remove the need to set Debug for DiagnosticLogger to work at all?
bitsandfoxes commentedon Apr 11, 2024
As in: If there is a
DiagnosticLogger present, log?vaind commentedon Apr 11, 2024
Yes, pretty much.
jamescrosswell commentedon Apr 12, 2024
We could/should be able to control everything via the
DiagnosticLevelright?sentry-dotnet/src/Sentry/SentryOptions.cs
Line 641 in 0711c17
So SentryOptions.Debug is a bit redundant... although lots of customers will definitely know about it so if we do phase it out, probably need to mark it as
Obsolete(and keep it as a proxy to setting theDiagnosticLevel) until some time in the distant future (e.g. v5.0).Ideally we'd change the default value of
SentryOptions.DiagnosticLeveltoWarningas well... if people did setDebug = true, that would be (for the time being) an alternative way of settingSentryOptions.DiagnosticLevel = SentryLevel.Debugvaind commentedon Apr 17, 2024
it is but it's common to all SDKs so I would keep it, just use it to set the diagnostic level to Debug, instead of the default warning - as you're suggesting. The main gripe I have with the current code is the need to enable Debug to get any logs, even warnings
jamescrosswell commentedon Apr 18, 2024
Yeah fair enough. What do you think should happen if people set
SentryOptions.Debug = falsethough (which is the default value for that property)? Would that set the DiagnosticLevel toError?If we leave SentryOptions.Debug in the SDK, we have to deal with 10 different combinations of those two settings (Debug x DiagnosticLevel) as well as the order that these are set in (if people are using the Options callback) and which one takes precedence (if people are just passing in an options object). Not saying that can't be done but we'd need to decide how it should work first right?
We could do it a little bit like EnableTracing, which implicitly sets TraceSampleRate to 1.0 or 0.0 depending on whether it's set to true or false. EnableTracing is a
bool?though (not abool). So maybe we could changeSentryOptions.Debugto be abool?and do something similar there? If it's true, we setSentryOptions.DiagnosticLevelto Debug... if it's false we set it toError... or alternatively people can get more granular by settingSentryOptions.DiagnosticLeveldirectly. If they've set both though, which one takes precedence?vaind commentedon Apr 18, 2024
I've checked what sentry-java and sentry-dart do and they set the default diagnostic logger if debug is true, otherwise leave a noop logger. I think the idea here was the same (since the .net sdk precedes these two, I think) but somehow the part when a user sets their own logger is messed up here. Maybe we should only fix that instead of touching diagnostic levels.
vaind commentedon Jan 8, 2025
This came up again and there're now develop docs describing how this should be handled.
https://develop.sentry.dev/sdk/expected-features/#debug-mode
13 remaining items
vaind commentedon Jul 16, 2025
Just to clarify - some SDKs, for the same reason, changed the default log level
As is, I still believe the SDK should be printing warnings and errors by default. They provide important information and visibility to issues at runtime (for example, not sending any events to Sentry because of misconfiguration)
Flash0ver commentedon Jul 16, 2025
Hmmm ... well ... perhaps there is a way we can figure out for this to be helpful, yet not too noisy, as discussed in #4330.
Is this what you have in mind? (also considering the dev docs)
SentryOptions.Debug = trueDiagnosticLevel = SentryLevel.Debugper defaultSentryOptions.Debug = false'$(Configuration)'=='DEBUG', andSentryOptions.Debugis not explicitly setSentryLevel.WarningorSentryLevel.ErrororSentryLevel.Fatalas minimum levelA couple of thoughts:
[ModuleInitializer]and only works fornet5.0.NET 9-compatible version of the CompilerWhich leads me to a question:
sentry-unitynorsentry-powershellsentry-unity: I guess in Unity we could inject something similar to the consuming game, to detect a Debug-Configuration, couldn't we @bitsandfoxes?sentry-powershell: for PowerShell I have never thought about the difference of a Debug-vs-Release buildTL;DR;
Questions about behavior between older .NET Target Frameworks and newer .NET Target Frameworks;
and between C#, VB and F#;
and between
sentry-dotnet,sentry-powershellandsentry-unity.vaind commentedon Jul 16, 2025
It is perfectly fine to have things only work for a subset of SDK users, if the subset is reasonably large. I don't know how many VB or F# consumers we have (I am not aware whether we track this) but I'm quite convinced it's safe to assume most would be using c#. If we can improve developer-experience for 90+ % of new users with a reasonable effort, I believe we should do that, but you're the maintainer of the SDK so it's up to you.
Flash0ver commentedon Jul 23, 2025
@jamescrosswell and I reiterated, and we're thinking about updating a prominent sample, or maybe most/all of the samples, in the docs with