Add Serilog integration #118
Conversation
|
Thanks for this @Mitch528 Could you please add a sample project? Also, I wonder, how does it work when combined with ASP.NET Core? |
|
Done. I haven't tested it with ASP.NET Core, but I'm not sure if it would combine well with it since the AspNetCore package uses Sentry.Extensions.Logging and I'm guessing both would attempt to send the same event to Sentry if used together. |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFrameworks>net462</TargetFrameworks> |
There was a problem hiding this comment.
@bruno-garcia the other test projects, except for the Log4Net one, target netcoreapp2.1;netcoreapp2.0;net462. Should these projects do the same?
|
@bruno-garcia Yep, added. |
There was a problem hiding this comment.
Done.
I haven't tested it with ASP.NET Core, but I'm not sure if it would combine well with it since the AspNetCore package uses Sentry.Extensions.Logging and I'm guessing both would attempt to send the same event to Sentry if used together.
The SDK has a built in de-duplication processor but it wouldn't be needed in this case. Serilog removes the MEL logging processor when it adds itself.
As I mentioned in another review comment, MEL has 2 levels which configure what is sent as event and what is stored as breadcrumbs. It would be great if the Serilog integration also had this behavior. This mean that when logging Informational messages, by default, that data would only be stored as breadcrumbs and sent with the following events. Sentry is not really a logging tool and simply sending all log messages to Sentry is not the way go to IMO.
| Hub = hubGetter; | ||
| } | ||
|
|
||
| public void Emit(LogEvent logEvent) |
There was a problem hiding this comment.
Looks like Emit is simply raising an event to Sentry with any log message created.
It would be nicer to enable a support like the one done for Microsoft.Extensions.Logging which have 2 log levels switches. One controls what is sent to sentry (creating an event) and one controls what should be added as breadcrumb.
This is great because we can store logs messages like Information as breadcrumbs and in the event of a LogError all the Information from that same scope are sent out with the the error event.
There was a problem hiding this comment.
Not entierly accurate. If you take a look here:
sentry-dotnet/src/Sentry.Serilog/SentrySinkExtensions.cs
Lines 12 to 19 in 6289801
The restrictedToMinimumLevel option sets the minimum level for which Emit is called by Serilog. I'm not really sure what the best way would be to have a minimum breadcrumb level, since Serilog will not call Emit if the aforementioned option is, for example, set to Error and the miniumum breadcrumb level is set to Info.
We could override restrictedToMinimumLevel and set it to the lowest level and then add our own switch that checks it within Emit, but that seems to be a bit counterintuitive I think.
There was a problem hiding this comment.
restrictedToMinimumLevel is the minimal level to either event or breadcrumb then.
For example: restrictedToMinimumLevel = Info, we would capture breadcrumbs (for Info or higher) and send events (error or higher)
If restrictedToMinimumLevel is Warning, we'd only capture breadcrumbs for Warnings or higher, and send events Error or higher.
Finally if restrictedToMinimumLevel is Error, then breadcrumbs would be tracked only for Error or higher as well as sending events.
A logging integration with Sentry that sends all Log.. to Sentry doesn't make much sense. Sentry is not a logging tool and flooding it with events that are not grouped together create a storm of noise (i.e: Notifications).
Keeping the Info/Warn logs around (breadcrumbs) for a while to be sent in case there's an Error is, in my opinion, the best approach as it gives you the context you need to troubleshoot an issue.
No description provided.