-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Add support for Serilog.Formatting.ITextFormatter #998
Add support for Serilog.Formatting.ITextFormatter #998
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
Before we can merge this could you please add a test that uses the TextFormatter
?
There's also a sample in the repo: https://github.com/getsentry/sentry-dotnet/tree/main/samples/Sentry.Samples.Serilog
And one with ASP.NET Core: https://github.com/getsentry/sentry-dotnet/tree/main/samples/Sentry.Samples.AspNetCore.Serilog
Would you please also use it in one of the samples to help show its usage?
Codecov Report
@@ Coverage Diff @@
## main #998 +/- ##
==========================================
- Coverage 81.26% 79.51% -1.75%
==========================================
Files 184 184
Lines 5893 5903 +10
Branches 1452 1454 +2
==========================================
- Hits 4789 4694 -95
- Misses 689 801 +112
+ Partials 415 408 -7
Continue to review full report at Codecov.
|
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
The only thing is that as a breaking change this should land on our next major.
@Tyrrrz thoughts?
Oh @Genteure could you please add a changelog entry? |
Sure. Which version should I put this change under? From |
Right, I don't know why it didn't write the comment here but it should be something like
|
Looks good, I only have one nit |
Thanks @Genteure 🎉 |
This feature is needed because sometimes a identifier of some kind (example: user id, task id) is added via
Serilog.ILogger.ForContext()
so it's not visible in the log message body.Currently there's no option to control how sentry formats received log events.
Most Serilog sinks that output texts have a
Serilog.Formatting.ITextFormatter formatter
orstring outputTemplate
argument so additional information can be added to the result log text.This pull request is adding more arguments to public methods thus should be considered as a breaking change.