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

feat: Add support for Serilog.Extensions.Logging and NLog.Extensions.Logging #1860

Merged
merged 7 commits into from Aug 23, 2023

Conversation

chynesNR
Copy link
Member

@chynesNR chynesNR commented Aug 23, 2023

Previously, if we detected Microsoft.Extensions.Logging and another logger, we would only instrument MEL, in order to avoid double collection of logs. This turned out to be a flawed approach, because MEL does not store any context data. It relies on the underlying logger to handle it. So now, if we detect that scenario, we disable MEL instrumentation. Only if we don't find another logger that we know how to instrument, will we instrument MEL.

In order for us to collect context data from an ILogger via MEL, the logger must implement IExternalScopeProvider (specifically the ForEachScope function). Scope data can be any object, but we will only collect it if it's a key-value pair (or collection of key-value pairs). This is how NLog and Serilog implement their scope provider, so this seems like a reasonable restriction.

I also found that NLog treats context data and scope data differently. We weren't recording NLog's scope data at all before; now we are.

Addresses #1859

Author Checklist

  • Unit tests, Integration tests, and Unbounded tests completed

Reviewer Checklist

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

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.

Looks good! I appreciate all of the comments to help understand what is going on. 👍

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Merging #1860 (3c7c667) into main (6524915) will decrease coverage by 0.13%.
Report is 3 commits behind head on main.
The diff coverage is 44.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
- Coverage   83.57%   83.44%   -0.13%     
==========================================
  Files         389      402      +13     
  Lines       24504    24670     +166     
==========================================
+ Hits        20478    20586     +108     
- Misses       4026     4084      +58     
Files Changed Coverage Δ
.../NewRelic.Agent.Extensions/Logging/LogProviders.cs 0.00% <0.00%> (ø)
....Agent.Extensions/Reflection/VisibilityBypasser.cs 89.26% <100.00%> (+0.21%) ⬆️

... and 21 files with indirect coverage changes

@chynesNR chynesNR merged commit ad24201 into main Aug 23, 2023
69 checks passed
@chynesNR chynesNR deleted the feature/mel-updates branch August 23, 2023 22:17
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.

None yet

4 participants