search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
605 stars 206 forks source link

Serilog LoggingLevelSwitch #2150

Open sapsari opened 1 year ago

sapsari commented 1 year ago

Problem Statement

Due to current API design, we cannot add a Serilog level switch to Sentry sink.

I want something like this:

const Serilog.Events.LogEventLevel sentryMinLevel = Serilog.Events.LogEventLevel.Warning;
static LoggingLevelSwitch sentryLevelSwitch;

var config = new LoggerConfiguration().MinimumLevel.Debug();

config = config.WriteTo.Sink(_sentrySink, sentryMinLevel, sentryLevelSwitch);

Either make class SentrySink public or add an additional argument (of type LoggingLevelSwitch ) to the extension methods.

Solution Brainstorm

No response

mattjohnsonpint commented 1 year ago

For context, here's the method we're talking about:

https://github.com/getsentry/sentry-dotnet/blob/f16d8cfed0ba55d0babe9760e110dd0ad8494537/src/Sentry.Serilog/SentrySinkExtensions.cs#L319-L339

Adding an overload that would take a LoggingLevelSwitch parameter and pass it along to Serilog is trivial. That would allow the switch to control the minimum log level of data sent to the Sentry Serilog sink.

However, that won't necessarily provide the desired effect. The MinimumBreadcrumbLevel and MinimumEventLevel values are also passed in the options passed to Sentry's Init method. Thus, changing the level via the switch would not actually change the level Sentry uses.

We would need to go further into the Sentry.Extensions.Logging internals, and provide a mechanism to change the logging level(s) dynamically without re-initalizing the SDK. Then we could wire the switch up to that.

TBH, it's a lot of work for not a lot of value. I'll keep this on the backlog, but we'll likely not get to it any time soon. A PR would be welcome, however. Thanks.

aiyervenkat commented 1 year ago

G'Day Matt,

Hope you are well. I am new to this. I have forked the repo. You mentioned a PR would be welcome. Were referring to just adding the Overload or broader fix by going deeper into Sentry.Extensions.Logging? I would appreciate the clarification.

Thanks Andy

jamescrosswell commented 1 year ago

Hi @aiyervenkat , Matt no longer works with Sentry but I may be able to help. I'm trying to understand what the original request was for however. Sentry's Serilog integration already let's you configure a minimum event level that is used to determine whether Serilog events get passed on to Sentry or not.

What is it you'd like to do that you can't currently do with Serilog and Sentry?

sapsari commented 1 year ago

Hi @aiyervenkat , Matt no longer works with Sentry but I may be able to help. I'm trying to understand what the original request was for however. Sentry's Serilog integration already let's you configure a minimum event level that is used to determine whether Serilog events get passed on to Sentry or not.

What is it you'd like to do that you can't currently do with Serilog and Sentry?

Level switches lets you modify MinimumEventLevel after initialization

jamescrosswell commented 1 year ago

Level switches lets you modify MinimumEventLevel after initialization

OK thanks @sapsari, and in what scenario(s) are you wanting to do that? Why would you want to dynamically change that threshhold?

sapsari commented 1 year ago

Level switches lets you modify MinimumEventLevel after initialization

OK thanks @sapsari, and in what scenario(s) are you wanting to do that? Why would you want to dynamically change that threshhold?

I want user to enable/disable error tracking via options at runtime. App's serilog has four sinks, only sentry sink doesn't support level switches

bitsandfoxes commented 1 year ago

Level switches lets you modify MinimumEventLevel after initialization

OK thanks @sapsari, and in what scenario(s) are you wanting to do that? Why would you want to dynamically change that threshhold?

I want user to enable/disable error tracking via options at runtime. App's serilog has four sinks, only sentry sink doesn't support level switches

If it's really about enable/disable does closing the SDK work as a workaround for you?

jamescrosswell commented 1 year ago

Another option might be the SetBeforeSend callback... could that work? You'd have to set a flag somewhere indicating whether error tracking was enabled and then check this flag in the BeforeSend callback. If the flag wasn't set, the callback would return null (i.e. discard the event).

bruno-garcia commented 1 year ago

Can we add LoggingLevelSwitch to the SentrySerilogOptions?

jamescrosswell commented 1 year ago

Can we add LoggingLevelSwitch to the SentrySerilogOptions?

See Matt's message earlier in this thread.