Support Auto Debug Mode

[vaind]

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.

[bitsandfoxes]

Looks like Debug controls whether the SDK adds its own ConsoleLogger as DiagnosticLogger if none is present.

sentry-dotnet/src/Sentry/SentryOptions.cs

Lines 1636 to 1645 in dfe208f

	internal void SetupLogging()
	{
	if (Debug)
	{
	if (DiagnosticLogger == null)
	{
	DiagnosticLogger = new ConsoleDiagnosticLogger(DiagnosticLevel);
	DiagnosticLogger.LogDebug("Logging enabled with ConsoleDiagnosticLogger and min level: {0}",
	DiagnosticLevel);
	}

Instead, I'd expect the DiagnosticLogger property to always be reachable. It already has a check which log levels should be printed.

You can set your own logger and that works independently whether Debug is enabled or not.

[vaind]

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 Debug in which case it will print not only warnings but also all the debug messages.

[vaind]

Actually, no, you cannot set a diagnostic logger.

sentry-dotnet/src/Sentry/SentryOptions.cs

Lines 651 to 653 in 6274944

	public IDiagnosticLogger? DiagnosticLogger
	{
	get => Debug ? _diagnosticLogger : null;

Can we just remove the need to set Debug for DiagnosticLogger to work at all?

[bitsandfoxes]

Can we just remove the need to set Debug for DiagnosticLogger to work at all?

As in: If there is a DiagnosticLogger present, log?

[vaind]

Yes, pretty much.

[jamescrosswell]

We could/should be able to control everything via the DiagnosticLevel right?

sentry-dotnet/src/Sentry/SentryOptions.cs

Line 641 in 0711c17

public SentryLevel DiagnosticLevel { get; set; } = SentryLevel.Debug;

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 the DiagnosticLevel) until some time in the distant future (e.g. v5.0).

Ideally we'd change the default value of SentryOptions.DiagnosticLevel to Warning as well... if people did set Debug = true, that would be (for the time being) an alternative way of setting SentryOptions.DiagnosticLevel = SentryLevel.Debug

[vaind]

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 the DiagnosticLevel) until some time in the distant future (e.g. v5.0).

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]

The main gripe I have with the current code is the need to enable Debug to get any logs, even warnings

Yeah fair enough. What do you think should happen if people set SentryOptions.Debug = false though (which is the default value for that property)? Would that set the DiagnosticLevel to Error?

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 a bool). So maybe we could change SentryOptions.Debug to be a bool? and do something similar there? If it's true, we set SentryOptions.DiagnosticLevel to Debug... if it's false we set it to Error... or alternatively people can get more granular by setting SentryOptions.DiagnosticLevel directly. If they've set both though, which one takes precedence?

[vaind]

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]

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