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

Sentry.Serilog throws if a DSN is not passed even though SentrySdk should already be initialized #2866

Closed
kanadaj opened this issue Nov 19, 2023 · 4 comments · Fixed by #2883
Assignees
Labels
Bug Something isn't working
Milestone

Comments

@kanadaj
Copy link
Contributor

kanadaj commented Nov 19, 2023

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.0

OS

Any (not platform specific)

SDK Version

4.0.0-beta.1

Self-Hosted Sentry Version

No response

Steps to Reproduce

  1. Add Sentry.Serilog and Sentry.AspNetCore to the app
  2. Init Sentry with `builder.ConfigureWebHostDefaults(webBuilder => webBuilder.UseSentry(...));```
  3. Try adding serilog with builder.Services.CongureLogging()
  4. Use .WriteTo.Sentry() with no arguments

Expected Result

The Serilog .WriteTo.Sentry() call shouldn't throw if SentrySdk is already initialized in the HostBuilder with .UseSentry()

Actual Result

Throws during startup with ArgumentNullException

@kanadaj kanadaj added Bug Something isn't working Platform: .NET labels Nov 19, 2023
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Nov 19, 2023
@bruno-garcia
Copy link
Member

bruno-garcia commented Nov 19, 2023

Should be an easy fix with SentrySdk.IsEnabled before throwing.

We should also add to the 4.0 changelog at the top on breaking changes. That if the SDK is disabled and null is provided, we'll throw. To use "" instead.

@kanadaj
Copy link
Contributor Author

kanadaj commented Nov 19, 2023

Do note that passing an empty string to WriteTo.Sentry() doesn't actually fix this since it uses the following code:

        if (!string.IsNullOrWhiteSpace(dsn))
        {
            sentrySerilogOptions.Dsn = dsn;
        }

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Nov 19, 2023
@jamescrosswell jamescrosswell added this to the 4.0.0 milestone Nov 19, 2023
@jamescrosswell
Copy link
Collaborator

@jamescrosswell
Copy link
Collaborator

jamescrosswell commented Nov 22, 2023

@kanadaj I think in this scenario, where you have already initialized Sentry via the generic host (and a call to UseSentry), the best option would be to pass a value of false in the initializeSdk argument when hooking Sentry up to Serilog. So something like:

builder.Host.UseSerilog(new LoggerConfiguration()
    .WriteTo.Sentry(initializeSdk: false)
    .CreateLogger());

That way, even though you're not supplying a DSN, it won't try to use that (null) value to initialise the SDK.

I've made a change so that you can now also provide the disabled DSN and it won't throw... but in that scenario you'd be initializing Sentry with a DisabledHub and nothing would get sent through to Sentry (which I don't think is what you want).

I've also created another ticket to see if we can get smart about how we set the default value of initializeSdk... If a call had already been made SentrySdk.Init or UseSentry, for example, we could default this to false and conversely if those methods hadn't already been called we could leave the default of true... which would save you from having to set this parameter manually yourself if you'd already initialized Sentry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants