Add ApiExceptionDestructurer for Refit.ApiException (Refit NuGet) #398
Conversation
|
Pull request based on existing implementations of Also the NuGet links in the documentation are for the moment simply placeholders, I wasn't sure what to do for them. |
Serilog.Exceptions.sln
Outdated
| @@ -1,7 +1,7 @@ | |||
| | |||
| Microsoft Visual Studio Solution File, Format Version 12.00 | |||
| # Visual Studio Version 17 | |||
| VisualStudioVersion = 17.0.31423.177 | |||
| # Visual Studio Version 16 | |||
There was a problem hiding this comment.
Yes, I put it back to 17 for you.
| #pragma warning disable CA1062 // Validate arguments of public methods | ||
| var apiException = (ApiException)exception; | ||
| propertiesBag.AddProperty(nameof(ApiException.Uri), apiException.Uri); | ||
| propertiesBag.AddProperty(nameof(ApiException.Content), apiException.Content); |
There was a problem hiding this comment.
I've not used Refit before. Does ApiException.Content effectively contain the response body? That could potentially be very large and maybe something you wouldn't want to log by default.
There was a problem hiding this comment.
Yes this is the HTTP response body. Yes I hesitated on this property, this could potentially be very large. I could add an option in the constructor bool destructureContent with a default of false ?
There was a problem hiding this comment.
Probably content should be destructured, that's fine. Maybe we should just limit the payload to some reasonable limit (like 2kb) and let user configure otherwise.
There was a problem hiding this comment.
All logging frameworks used by ASP.NET omit the content. We should do the same by default and provide an option to enable it.
There was a problem hiding this comment.
I made it optional (default disabled). Let me know if this is ok and I will add some tests for it.
Source/Serilog.Exceptions.Refit/Destructurers/ApiExceptionDestructurer.cs
Outdated
Show resolved
Hide resolved
Complete tests for optional parameters in ApiExceptionDestructurer constructor.
README.md
Outdated
| ```csharp | ||
| .Enrich.WithExceptionDetails(new DestructuringOptionsBuilder() | ||
| .WithDefaultDestructurers() | ||
| .WithDestructurers(new[] { new ApiExceptionDestructurer(destructureCommonExceptionProperties = false) })) |
There was a problem hiding this comment.
Just asking, shouldn't it be destructureCommonExceptionProperties: false
There was a problem hiding this comment.
Absolutely, well spotted. Now corrected, also added docmentation for the other optional constructor parameter.
Source/Serilog.Exceptions.Refit/Serilog.Exceptions.Refit.csproj
Outdated
Show resolved
Hide resolved
Source/Serilog.Exceptions.Refit/Destructurers/ApiExceptionDestructurer.cs
Outdated
Show resolved
Hide resolved
|
LGTM. |
RehanSaeed
added
enhancement
resolves #396