-
Notifications
You must be signed in to change notification settings - Fork 290
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
Populate required field Message with n/a if it is empty #2857
Populate required field Message with n/a if it is empty #2857
Conversation
@microsoft-github-policy-service agree |
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, this looks good!
Will ask one more owner to review before merge, as we are adding limited additions (including fixes) to this repo.
Thanks @cijothomas, I've pushed a further change to move |
...t/Microsoft.ApplicationInsights.Tests/Extensibility/Implementation/ExceptionConverterTest.cs
Show resolved
Hide resolved
...t/Microsoft.ApplicationInsights.Tests/Extensibility/Implementation/ExceptionConverterTest.cs
Show resolved
Hide resolved
....ApplicationInsights/Extensibility/Implementation/External/ExceptionDetailsImplementation.cs
Outdated
Show resolved
Hide resolved
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.
please address comments
…y method to check for null/whitespace
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.
LGTM
@TimothyMothra What are the next steps to get this merged/released? Let me know if you need anything from me 🙂 |
Merged! |
So, half a year since this fix was merged (and even longer since the latest release for this library), maybe a good time for a release? :) |
@TimothyMothra / @cijothomas any chance of a release? We've just spent an entire day chasing this bug down only to find it was found in 2019, fixed 7 months ago and still not released... |
Fix Issue #1066.
Changes
Application Insights rejects exceptions that have empty messages. This means that some exceptions are lost, making investigation of runtime issues difficult. I noticed this when investigating an issue in a .NET 8 API, where a
CosmosException
for a conflict occurred, was logged on the console but wasn't logged in App Insights.There was previously a PR #2697 that never got merged due to lack of tests.
Original example from the other PR:
Original response from the other PR:
Checklist
For significant contributions please make sure you have completed the following items:
The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.
Notes for authors: