Skip to content

fix(log4net): send event properties as log attributes on log4net 3.x - #5559

Open
jamescrosswell wants to merge 4 commits into
mainfrom
fix/log4net-3x-properties-5557
Open

jamescrosswell wants to merge 4 commits into
mainfrom
fix/log4net-3x-properties-5557

Conversation

@jamescrosswell

@jamescrosswell jamescrosswell commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Closes #5557

Summary

On log4net 3.x, SentryAppender silently dropped every event property from structured logs — no property.* attribute was ever attached to a SentryLog, with no exception and no warning.

log4net.Util.PropertiesDictionary gained IDictionary<string, object?> in 3.x, and that changed what its non-generic IEnumerable yields: KeyValuePair<string, object?> instead of DictionaryEntry. Sentry.Log4Net is compiled against its 2.0.12 floor and walks exactly that non-generic enumerator, so property is DictionaryEntry matched nothing and the loop body never ran.

The fix reads through GetKeys() + the indexer, the way the sibling SentryEvent path in GetLoggingEventProperties already 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, while Sentry.Log4Net.Tests stays 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 InternalsVisibleTo entries 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, and ApiApprovalTests is green.

🤖 Generated with Claude Code

jamescrosswell and others added 2 commits September 10, 2026 14:09
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>
Comment thread test/Sentry.Log4Net.V3.Tests/Sentry.Log4Net.V3.Tests.csproj Outdated
jamescrosswell and others added 2 commits September 10, 2026 15:29
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

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.76%. Comparing base (0004ef8) to head (bd94d96).
⚠️ Report is 7 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jamescrosswell
jamescrosswell marked this pull request as ready for review September 14, 2026 00:18
@github-actions github-actions Bot added the risk: medium PR risk score: medium label Sep 14, 2026
Comment thread src/Sentry.Log4Net/SentryAppender.Structured.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sentry.Log4Net silently drops log property attributes on log4net 3.x

1 participant