feat: add Serilog integration
#4462
Conversation
|
|
@sentry review |
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
|
|
||
| private void CaptureStructuredLog(IHub hub, LogEvent logEvent, string formatted, string? template) | ||
| { | ||
| var traceHeader = hub.GetTraceHeader() ?? SentryTraceHeader.Empty; |
There was a problem hiding this comment.
Is the ParentSpanId required in the protocol for StructuredLogging? An empty SpanId probably isn't that useful otherwise eh?
There was a problem hiding this comment.
No ... it's an optional "Default Attribute" ... the trace_id is required.
Oops ... I made this mistake in the Sentry-SDK and Microsoft.Extensions.Logging integrations, too.
I'll create a follow-up PR to fix this issue in all integrations.
|
Thank you for your efforts here. 🙏 Is there a preview band we can try this out, perhaps? I have two projects that would really benefit from having this in place. Thank you for your continued efforts. |
|
Looking forward to when this gets out, would really benefit my team |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4462 +/- ##
==========================================
+ Coverage 73.42% 73.50% +0.08%
==========================================
Files 479 480 +1
Lines 17506 17587 +81
Branches 3480 3505 +25
==========================================
+ Hits 12853 12928 +75
+ Misses 3774 3773 -1
- Partials 879 886 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| private bool IsEnabled(LogEvent logEvent) | ||
| { | ||
| var options = _hubAccessor()?.GetSentryOptions(); |
There was a problem hiding this comment.
| var options = _hubAccessor()?.GetSentryOptions(); | |
| var options = _hubAccessor().GetSentryOptions(); |
_hubAccessor() is never null
This also looks unecessary:
sentry-dotnet/src/Sentry.Serilog/SentrySink.cs
Lines 100 to 101 in 02defb5
|
|
||
| return logEvent.Level >= _options.MinimumEventLevel | ||
| || logEvent.Level >= _options.MinimumBreadcrumbLevel | ||
| || options?.Experimental.EnableLogs is true; |
There was a problem hiding this comment.
I was a bit confused by this... so we don't filter any structured logs by Log Level - everything gets captured (all the way down to SentryLogLevel.Trace)?
Worth adding a code comment explaining why this is the case. I think we inherit the log level filter from MEL so we don't need to implement anything explicitly ourselves right?
|
|
||
| Dictionary<string, string>? data = null; | ||
| if (exception != null && !string.IsNullOrWhiteSpace(formatted)) | ||
| var options = hub.GetSentryOptions(); |
There was a problem hiding this comment.
Maybe worth adding a note explaining why we fetch the options from the Hub here (rather than just using _options as we do above). I remember we had a conversation about this but I can't remember what the reason was anymore.
Changes
Add
Serilogintegration to Sentry Structured Logging.Docs
Dogfooding
getsentry/symbol-collector->SymbolCollector.Server