-
Notifications
You must be signed in to change notification settings - Fork 65
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
fix: fix regression in NLog local decoration when logging objects as parameters #1480
Conversation
Comments came from a misunderstanding about Visual Studio's display of an object when you mouse over it coming from ToString().
Also implement LogWithParam in other adapters even though we might not use them yet
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/NLogLogging/NLogWrapper.cs
Outdated
Show resolved
Hide resolved
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/NLogLogging/NLogWrapper.cs
Show resolved
Hide resolved
...edApplications/Common/MultiFunctionApplicationHelpers/MultiFunctionApplicationHelpers.csproj
Show resolved
Hide resolved
src/Agent/NewRelic/Agent/Extensions/Providers/Wrapper/NLogLogging/NLogWrapper.cs
Outdated
Show resolved
Hide resolved
Also removed TODO from MFAH.csproj. I want to make that correction as part of a larger set of improvements to how we associate CMF TFMs with library versions.
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.
Looks good!
src/Agent/NewRelic/Agent/Extensions/NewRelic.Agent.Extensions/Logging/LoggingHelpers.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.
LGTM!
Codecov Report
@@ Coverage Diff @@
## main #1480 +/- ##
==========================================
- Coverage 72.98% 72.98% -0.01%
==========================================
Files 408 408
Lines 25459 25462 +3
==========================================
+ Hits 18582 18583 +1
- Misses 6877 6879 +2
|
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.
Looks good! I'm glad to see we were able to identify and fix the issue in basic (non web) NLog.
Description
See bug #1459. PR #1393 improved NLog local decoration coverage, but broke logging of objects as parameters, e.g.:
_logger.LogDebug("Start Request: {@request}", request);
This PR fixes the issue and adds test coverage for the parameterized logging case.
Author Checklist
Reviewer Checklist