fix(log4net): send event properties as log attributes on log4net 3.x - #5559
Open
jamescrosswell wants to merge 4 commits into
Open
jamescrosswell wants to merge 4 commits into
jamescrosswell wants to merge 4 commits into
Conversation
log4net 3.x added IDictionary<string, object?> to PropertiesDictionary, which changed what its non-generic IEnumerable yields: KeyValuePair instead of DictionaryEntry. The appender is compiled against the 2.0.12 floor and walks that non-generic enumerator, so its `is DictionaryEntry` pattern silently matched nothing on 3.x and no property.* attribute was ever set. Read through GetKeys()/the indexer instead, as the sibling SentryEvent path already does - that API behaves identically on both majors, so the appender keeps working whether the consumer resolves log4net 2.x or 3.x and the package floor stays at 2.0.12. Adds Sentry.Log4Net.V3.Tests, which links the same appender tests and runs them against log4net 3.4.0. Without this fix it reproduces exactly the three reported failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The .slnf files are generated by scripts/generate-solution-filters.ps1, and Sentry-CI-Build-Linux-NoMobile.slnf is based on the committed .generated.NoMobile.slnx rather than Sentry.slnx. Without regenerating it, the Linux legs failed to restore with MSB4025: the filter included a project that was not in the solution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jamescrosswell
commented
Sep 10, 2026
The project name says what it is, and the rationale for it lives in the PR description. Also tightens the appender comment to state the constraint (the enumerator differs between majors) rather than narrate the bug that exposed it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sentry.Log4Net.V3.Tests is the regression mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5559 +/- ##
=======================================
Coverage 74.75% 74.76%
=======================================
Files 515 515
Lines 18963 18963
Branches 3694 3694
=======================================
+ Hits 14176 14177 +1
+ Misses 3909 3907 -2
- Partials 878 879 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jamescrosswell
marked this pull request as ready for review
September 14, 2026 00:18
jamescrosswell
commented
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5557
Summary
On log4net 3.x,
SentryAppendersilently dropped every event property from structured logs — noproperty.*attribute was ever attached to aSentryLog, with no exception and no warning.log4net.Util.PropertiesDictionarygainedIDictionary<string, object?>in 3.x, and that changed what its non-genericIEnumerableyields:KeyValuePair<string, object?>instead ofDictionaryEntry.Sentry.Log4Netis compiled against its 2.0.12 floor and walks exactly that non-generic enumerator, soproperty is DictionaryEntrymatched nothing and the loop body never ran.The fix reads through
GetKeys()+ the indexer, the way the siblingSentryEventpath inGetLoggingEventPropertiesalready does. That API behaves identically on both majors, so the appender is version-agnostic and the package floor stays at 2.0.12 — no new minimum for consumers still on log4net 2.x.Notes for review
New test project,
Sentry.Log4Net.V3.Tests. This is a runtime-only difference — nothing that builds against 2.x can catch it — so covering it needs the appender actually loaded against a 3.x assembly. The new project links the same two appender test files (Compile Include, not copies, so the two majors can't drift) and references log4net 3.4.0, whileSentry.Log4Net.Testsstays on 2.0.15. Both majors now run in CI. I confirmed the new project has teeth: reverting just the appender change reproduces exactly the three failures from the issue (52 passed / 3 failed), and both suites are green with it.The two Verify-based tests are deliberately not linked into the new project — their snapshots are keyed off the original file paths, so running them from two projects would have both writing the same files.
Three
InternalsVisibleToentries were needed for the new assembly (Sentry,Sentry.Log4Net,Sentry.Testing), following the pattern every other test project uses. These don't appear in the API approval snapshots, andApiApprovalTestsis green.🤖 Generated with Claude Code