Exception with redefined property(new modifier) stops enrichment altogether
domagojmedo opened this issue · 13 comments
Describe the feature
Not sure if this eve belongs here, but I'm having trouble getting this package to work with ApplicationInsights sink. I'm trying to get deconstructed Refit.ValidationApiException but no luck.
Any help would be appreciated
I am using Serilog.Exceptions with ApplicationInsights sink and it works fine.
You need to provide an example https://stackoverflow.com/help/minimal-reproducible-example, for us to check.
Moreover, Serilog.Exceptions is basically unrelated to any particular sink, its code is executed before sink code obviously.
Yea, my bad, it seems that issue is with Refit.ValidationApiException. It's not getting deconstructed. I'll try to create an example
I'm having trouble finding a public API that will return that exception.
In this repo everything works when ApiException is caught, but if ValidationApiException is caught then nothing gets deconstructed, not even properties from the base ApiException class.
I'll try to find a public API that throws ValidationApiException but at the moment this is the best I could come up with.
In the meantime I changed my code to serialize Content manually and log it.
catch (ValidationApiException apiException)
{
var validationError = JsonSerializer.Serialize(apiException.Content);
_logger.LogError(apiException, "Validation error occured while calling external API {validationError}", validationError);
}
You don't need public API, just create exception manually then log it and verify result in the log output.
I tried but had issues because it required APiException in constructor. Switched to using reflection now.
Repo is updated, you can see that Content is set to some ProblemDetails object, but nothing is output in console.
If you uncomment ThrowApiException and comment that method you will see that it works as intended
Took me a while to understand what is happening and I found out that this particular exception somehow declares property Content twice. Serilog.Exceptions fails silently in this case so you do not get any enrichment.
I will fix it tomorrow and let you know.
Ok, it just overrides the definition of Content with new keyword.
If someone has used the new keyword on a property, they are deliberately hiding the one the base class. I'm not really sure we should we be retrieving both. In this case, the derived class looks like it gives us a better representation of the content (ProblemDetails) than the base class (String). Thoughts @krajek?
@RehanSaeed yes, agreed 100%, next PR will tackle that problem.
Just to reiterate: this PR is just a safety mechanism, the last line of defense, but not the only one.
I assume that robustness(meaning in that case "providing value to the library user" even in the unexpected scenario) is so important for our library that such a mechanism is justifiable.
I will start the next PR just after this is merged.
@RehanSaeed when can we expect new version with this fix?
@krajek Do you want to put in another PR with further improvements? E.g. using ConcurrentDictionary?