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 issue #2261 #2315

Merged
merged 1 commit into from Mar 8, 2024
Merged

Fix issue #2261 #2315

merged 1 commit into from Mar 8, 2024

Conversation

lowell-trimble
Copy link
Contributor

@lowell-trimble lowell-trimble commented Mar 7, 2024

BEGIN_COMMIT_OVERRIDE
fix: Fix a context data capture when the Microsoft.Extensions.Logging console logger is used (thanks @lowell-trimble!) (#2261) (#2315)
END_COMMIT_OVERRIDE

Description

Fixing issue #2261. When there are multiple logging providers registered, only the last provider will have the ExternalScopeProvider set to a non-null value. Because of this, new relic's logic to pull the first ScopeLogger from the array causes context data to be missed. The simple fix is to simply grab the last value in the array.

Author Checklist

  • Unit tests, Integration tests, and Unbounded tests completed
  • Performance testing completed with satisfactory results (if required)

Reviewer Checklist

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

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2024

CLA assistant check
All committers have signed the CLA.

@tippmar-nr tippmar-nr added the community To tag external issues and PRs label Mar 7, 2024
@nrcventura
Copy link
Member

@lowell-trimble Thank you for submitting this PR. The link to the LoggerFactory source that you shared was helpful in explaining the solution (so that we do not need to walk the list to find the first item with an external provider).

I think that the existing integration tests should be sufficient for this PR and I am running them locally to confirm that everything is still working as expected.

Copy link
Member

@nrcventura nrcventura left a comment

Choose a reason for hiding this comment

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

The changes look good and the tests pass locally.

@nrcventura nrcventura merged commit f8422d6 into newrelic:main Mar 8, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community To tag external issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants