Track exceptions in Application Insights publisher #302
Conversation
Previous version ignored reporting any exception contained in reports. Most of the Xabaril health checks do send a value for HealthCheckResult.Exception when the check fails, but they weren't being reported to App Insights. Note that there is lot of room for improvements on this class, so far I've only added a simple additional method to track exceptions.
src/HealthChecks.Publisher.ApplicationInsights/ApplicationInsightsPublisher.cs
Show resolved
Hide resolved
| return Task.CompletedTask; | ||
| } | ||
| private void TrackExceptions(HealthReport report, TelemetryClient client) |
There was a problem hiding this comment.
If the health report contains exception, you can save also detailed report and exception? probably detailed and generalized report will contain exception information without create any other method
There was a problem hiding this comment.
That's a good question. Initially, I was going to TelemetryClient.TrackException only in the detailed report, but I think it's information you would like to have even in the summary report, that's why I've added the TrackExceptions method.
Also, note that for detailed report you could choose to not report healthy results; it probably doesn't make much sense but it could happen that a report is healthy but it also includes an exception and this is why the TrackExceptions method is called regardless you choose a summary or detailed report.
|
|
||
| public ApplicationInsightsPublisher(string instrumentationKey = default, bool saveDetailedReport = false, bool excludeHealthyReports = false) | ||
| public ApplicationInsightsPublisher(string instrumentationKey = default, bool saveDetailedReport = false, bool excludeHealthyReports = false, bool trackExceptions = true) |
There was a problem hiding this comment.
How you can change TrackException parameters? probably you need to add a new overload or default parameter on ApplicationInsightsHealthCheckBuilderExtensions.cs
There was a problem hiding this comment.
You are correct, I was going to add a default on ApplicationInsightsHealthCheckBuilderExtensions. Again, the plan is adding a default bool trackExceptions = true rather than false because I think having a report on exceptions happened is something one would like for default (at least I know it's what I wanted).
There was a problem hiding this comment.
I've add the parameter to the extension method in 0c2e389.
|
@unaizorrilla thanks for the comments! I've left some answers, and I'll be working on some of those suggestions. I'll also try to add a basic test for this new operation. |
This commit only includes tests for tracking exceptions in ApplicationInsightsPublisher. Tests for existing tracking methods are left until this one is approved or rejected. If approved I'll add the tests in a new pull request, as adding those is out of the scope of the current pull request Xabaril#302.
0c2e389 to
52d77d9
Compare
While at it add some simple unit tests for this extension method.
52d77d9 to
9a85635
Compare
|
@unaizorrilla I've made some changes and added some tests, I think it's ready for another round of review. Thank you! |
|
Hi @inkel I try to review this again today! |
|
I'm so sorry for delaying on review the PR! :-( I try to do ASAP |
|
@unaizorrilla don't worry about it! 😸 |
While at it add some simple unit tests for this extension method.
|
Hi @inkel I perform some changes in the pr and merge it to master, many thanks to contribute on this. My changes try to preserve your intention, track exception on ai also as part of telemetry information. But I consider to do only when you set detailed reports and not a separate method. Also I need to work more on unit test to improve our coverage... Thanks again to contribute and so sorry to do changes in your pr withut normal way |
|
@unaizorrilla hi, thanks! I'm more than ok with you making changes 😸 though for some reason GitHub is not showing those to me. Anyway, thanks for being open to receive, review and merge this PR ❤️
Would you like me to help you with that? One of the reasons I've added the tests for this publishe was that secretly I planned on submitting another PR that added more tests to it for the existing features 😊 |
This PR implements exception tracking on App Insights as described in #299.
My initial reaction was to add this only in the detailed report, but then I realized that I wanted to have the exceptions tracked, regardless of whether it's a detailed or summarized report.
While working on this I also found that it could be possible to refactor the code to be more DRY (e.g. creating the properties dictionary could be extracted to a method), and also that by using a mock
TelemetryChanneltests could be added for this publisher. I didn't include those changes because I think they're out of the scope of this PR description.