Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to DebugDiagnosticLogger when running in Visual Studio #3373

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

jamescrosswell
Copy link
Collaborator

@jamescrosswell jamescrosswell commented May 16, 2024

Resolves #3341

Approach

There are a couple of different ways we could solve this. One would be a CompositeLogger which could be initialised like this:

                DiagnosticLogger = new CompositeLogger(
                    new ConsoleDiagnosticLogger(DiagnosticLevel),
                    new TraceDiagnosticLogger(DiagnosticLevel)
                );

That would basically log to both. It's a bit of a hack since technically, you could instantiate each of the composite loggers with a different DiagnosticLevel. That could be guarded against by checking all the levels match in the constructor but it's not as elegant as I'd like.

Alternatively, we could default to a DebugDiagnosticLogger which uses Debug.WriteLine. That works in both Rider and Visual Studio.

It's a small change to the behavriour of the API. Anyone who wanted to retain the previous behaviour could explicitly set SentryOptions.DiagnosticLogger to be a ConsoleDiagnosticLogger.

In this PR, I've gone with that option.

@jamescrosswell jamescrosswell marked this pull request as ready for review May 17, 2024 08:57
@bitsandfoxes
Copy link
Contributor

I did a quick check with our Blazor.WASM sample and the logs in the browser seem to rely on the console. Ideally, we'd have some fallback to the console logger on platforms where we know it will go into the void.

@bitsandfoxes
Copy link
Contributor

I'm fine with the behaviour change in general because I assume that this is nothing that affects users in production
It enables debug functionality where it previously did not work.

@jamescrosswell
Copy link
Collaborator Author

I did a quick check with our Blazor.WASM sample and the logs in the browser seem to rely on the console.

Nice catch... tricky though. As we initialise Blazor using plain old SentrySdk.Init, I can't think of an easy way to determine, when initialising the SDK, if the app is a Blazor WASM app or not.

We could create a WASM integration with aWebAssemblyHostBuilder.UseSentry extension to initialise the SDK for WASM applications and set SentryOptions.DiagnosticLogger = new ConsoleDiagnosticLogger(SentryLevel.Debug) in that case. Given the popularity of Blazor/WASM, it might be worth it to make Sentry as usable as possible in this scenario. We could use that extension method to encapsulate some of the other things involved in correctly configuring the SDK for use with WASM as well.

Alternatively, we could just document/show in the WASM sample that the DiagnosticLogger needs to be set to something non-default (explaining that Debug.WriteLine doesn't work in WASM applications).

@bitsandfoxes
Copy link
Contributor

bitsandfoxes commented May 23, 2024

I tried using this all day and I think I changed my mind. I found this to be fairly irritating to have the SDK logs show up somewhere else than the rest of the program's logs.
And when running samples from the command line there were just no logs at all. So I'd have to manually set the ConsoleDiagnosticLogger in the options.

TLDR; this felt like a major breaking change.

@jamescrosswell jamescrosswell changed the title Default to TraceDiagnosticLogger when running in Visual Studio Default to DebugDiagnosticLogger when running in Visual Studio May 27, 2024
@jamescrosswell
Copy link
Collaborator Author

TLDR; this felt like a major breaking change.

We could revisit the initial solution but implement it more simply, like this:

public class TraceAndConsoleDiagnosticLogger : DiagnosticLogger
{
    public TraceAndConsoleDiagnosticLogger(SentryLevel minimalLevel) : base(minimalLevel)
    {
    }

    protected override void LogMessage(string message)
    {
        Trace.WriteLine(message);
        Console.WriteLine(message);
    }
}

It would create duplication if you had a console tracer configured, but I'd say that's a bit of an edge case (and we could guide people to change the default logger in those cases).

@bitsandfoxes
Copy link
Contributor

I think we agreed on the sync to try to log to both via the MAUI setup to catch those VS users?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No debug logs on MAUI when launching within Visual Studio
3 participants