-
Notifications
You must be signed in to change notification settings - Fork 64
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
Conversation
log4net, serilog, nlog, and microsoft.extensions.logging. Includes unit and integration tests.
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.
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.
src/Agent/NewRelic/Agent/Core/JsonConverters/LogEventWireModelCollectionJsonConverter.cs
Show resolved
Hide resolved
...nctionApplicationHelpers/NetStandardLibraries/LogInstrumentation/SerilogLoggingWebAdapter.cs
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.
I believe the logic for when to forward log events still needs some changes to be conformant to the specifications.
...nctionApplicationHelpers/NetStandardLibraries/LogInstrumentation/SerilogLoggingWebAdapter.cs
Show resolved
Hide resolved
...tionApplicationHelpers/NetStandardLibraries/LogInstrumentation/Controllers/TestController.cs
Outdated
Show resolved
Hide resolved
… logging-exception-attributes
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.
These changes look good to me!
tests/Agent/IntegrationTests/IntegrationTests/Logging/MetricsAndForwardingTests.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.
Changes look good to me. Looks good in the UI too. 👍
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.
Author Checklist
Reviewer Checklist