Skip to content
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

Implements sending exception attributes for log events #1228

Merged
merged 8 commits into from
Sep 8, 2022

Conversation

jaffinito
Copy link
Member

Description

Updates our RecordLogMessage api to accept a Func to get the exception data attached to the message, if present. All existing frameworks have been updated to capture this information.

  • Captures message, stack, and class of the exception
  • Stack is capture using our StackTraces.ScrubAndTruncate system so it will report similar results compared to error traces. This also enabled it to dig into the inner exceptions and pull together a complete stack.
  • Wiremodel updated to add new values. Stack is stored as a string, not string[], here since that is what the spec is asking for.
  • Unit tests (AgentWrapperApiTests) have been added.
  • Integration tests and our LoggingAdapter have been updated to take an exception for "Error" level logs instead of a string.
  • Integration test Assertions have been updated to check for for exception data in log lines, but only for "Error" level.

Author Checklist

Reviewer Checklist

  • Perform code review
  • Pull request was adequately tested (new/existing tests, performance tests)
  • Review Changelog

log4net, serilog, nlog, and microsoft.extensions.logging.
Includes unit and integration tests.
@jaffinito jaffinito marked this pull request as ready for review August 26, 2022 17:19
Copy link
Contributor

@JcolemanNR JcolemanNR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this will add a lot of value for customers calling log methods and supplying exceptions!

In addition to my inline comments requesting changes, I think we should add unit and integration test coverage for log events with a null/empty message and an exception per the spec.

@JcolemanNR JcolemanNR changed the title Implements sending exception attributes for: Implements sending exception attributes for log eventsf Aug 27, 2022
@JcolemanNR JcolemanNR changed the title Implements sending exception attributes for log eventsf Implements sending exception attributes for log events Aug 27, 2022
Copy link
Contributor

@JcolemanNR JcolemanNR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the logic for when to forward log events still needs some changes to be conformant to the specifications.

@JcolemanNR JcolemanNR self-requested a review September 7, 2022 21:02
JcolemanNR
JcolemanNR previously approved these changes Sep 8, 2022
Copy link
Contributor

@JcolemanNR JcolemanNR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me!

vuqtran88
vuqtran88 previously approved these changes Sep 8, 2022
@jaffinito jaffinito dismissed stale reviews from vuqtran88 and JcolemanNR via 480a7aa September 8, 2022 20:52
Copy link
Member

@nr-ahemsath nr-ahemsath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me. Looks good in the UI too. 👍

@jaffinito jaffinito merged commit d93f3c9 into main Sep 8, 2022
@jaffinito jaffinito deleted the logging-exception-attributes branch September 8, 2022 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for collecting exception details in log lines from in-agent log forwarding
5 participants