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

fix: fix regression in NLog local decoration when logging objects as parameters #1480

Merged
merged 10 commits into from
Mar 24, 2023

Conversation

nr-ahemsath
Copy link
Member

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

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

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.
@nr-ahemsath nr-ahemsath marked this pull request as ready for review March 24, 2023 14:40
chynesNR
chynesNR previously approved these changes Mar 24, 2023
Copy link
Member

@chynesNR chynesNR left a comment

Choose a reason for hiding this comment

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

Looks good!

jaffinito
jaffinito previously approved these changes Mar 24, 2023
Copy link
Member

@jaffinito jaffinito left a comment

Choose a reason for hiding this comment

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

LGTM!

@nr-ahemsath nr-ahemsath dismissed stale reviews from jaffinito and chynesNR via 41afbb7 March 24, 2023 16:09
@codecov-commenter
Copy link

Codecov Report

Merging #1480 (41afbb7) into main (2ed31c4) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...ewRelic.Agent.Extensions/Logging/LoggingHelpers.cs 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

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.

Looks good! I'm glad to see we were able to identify and fix the issue in basic (non web) NLog.

@nr-ahemsath nr-ahemsath merged commit a19addd into main Mar 24, 2023
@nr-ahemsath nr-ahemsath deleted the bugfix/nlog-ghissue-1459 branch March 24, 2023 20:40
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.

6 participants