Exceptionless selflog for serilog #4
Conversation
| @@ -45,6 +45,7 @@ public class ExceptionlessSink : ILogEventSink { | |||
| config.ServerUrl = serverUrl; | |||
|
|
|||
| config.UseInMemoryStorage(); | |||
| config.UseLogger(new SelfLogLogger()); | |||
There was a problem hiding this comment.
I don't think this should be included by default. We should add this to the docs, diagnostic messages shouldn't be included by default.
| @@ -66,7 +67,13 @@ public class ExceptionlessSink : ILogEventSink { | |||
| public ExceptionlessSink(Func<EventBuilder, EventBuilder> additionalOperation = null, bool includeProperties = true, ExceptionlessClient client = null) { | |||
| _additionalOperation = additionalOperation; | |||
| _includeProperties = includeProperties; | |||
| _client = client ?? ExceptionlessClient.Default; | |||
| if (client != null) { | |||
There was a problem hiding this comment.
Can we also revert this part, diagnostics shouldn't be included by default.
| using Serilog.Debugging; | ||
|
|
||
| namespace Serilog.Sinks.Exceptionless { | ||
| public class SelfLogLogger : IExceptionlessLog { |
There was a problem hiding this comment.
Will this class name conflict with other self log implementations? Should we prefix it?
|
@niemyjski You will not see diagnostics until you call Serilog.Debugging.SelfLog.Enable(Console.Out); |
| } | ||
| else { | ||
| _client = ExceptionlessClient.Default; | ||
| _client.Configuration.UseLogger(new SelfLogLogger()); |
There was a problem hiding this comment.
Do you think we should only do this on the default instance instead of every time? If you pass in your own client instance (you might be passing in ExceptionlessClient.Default), we won't wire up the self log? Maybe do _client = client ?? ExceptionlessClient.Default; and the following line call useLogger
There was a problem hiding this comment.
But in this case we will overwrite logging if user have already set up logging for the client.
Maybe we should apply logging if logger is null.
There was a problem hiding this comment.
Yeah, I think there is code in the client to detect the null logger.
There was a problem hiding this comment.
There isn't but the default logger will be https://github.com/exceptionless/Exceptionless.Net/blob/f0494c8015fe8ba76cdb11b9b017c6c1d68f8749/src/Exceptionless/Logging/NullExceptionlessLog.cs
There was a problem hiding this comment.
So you could do a type check and just set it and simplify the code a bit too to do the previous _client = client ?? ExceptionlessClient.Default;
|
@niemyjski Please have a look. Do we have better ways to understand that it is |
|
I'll take a deeper look in the morning. I could of swore we had a HasRegistration method. |
|
I found event better, we have |
|
@niemyjski Have a look at the last commit, i have used exactly this method :) |
PR For:
exceptionless/Exceptionless.Net#133