Repository navigation
Apply the documented EventRetentionPeriod setting to event retention - #5974
Open
johnsimons wants to merge 1 commit into
Open
johnsimons wants to merge 1 commit into
johnsimons wants to merge 1 commit into
Conversation
Existing installs may already have the older EventsRetentionPeriod spelling set, so keep honouring it as a fallback. The new EventRetentionPeriod name takes precedence when both are configured.
johnsimons
requested review from
warwickschroeder
and
a balanced review from Copilot
October 9, 2026 02:53
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The explicitly supported malformed-value fallback lacks regression coverage in both the persister and instance paths.
2 open findings
What changed in this PR
Aligns event retention with the documented setting while preserving compatibility with the legacy spelling.
Changes:
- Adds documented-key precedence with legacy fallback.
- Applies retention consistently across RavenDB, EF Core, and usage reporting.
- Documents and tests the configuration behavior.
| File | Description |
|---|---|
src/ServiceControl/Infrastructure/Settings/Settings.cs |
Adds legacy fallback for reporting. |
src/ServiceControl.UnitTests/Infrastructure/Settings/EventRetentionPeriodSettingsTests.cs |
Tests instance setting precedence. |
src/ServiceControl.Persistence/PersistenceConfiguration.cs |
Centralizes retention-setting resolution. |
src/ServiceControl.Persistence.Tests/EFCore/EventsRetentionConfigurationTests.cs |
Tests EF Core configuration. |
src/ServiceControl.Persistence.Tests.RavenDB/EventsRetentionConfigurationTests.cs |
Tests RavenDB configuration. |
src/ServiceControl.Persistence.RavenDB/RavenPersistenceConfiguration.cs |
Uses shared retention resolution. |
src/ServiceControl.Persistence.EFCore/Abstractions/EFPersistenceConfigurationBase.cs |
Uses shared retention resolution. |
docs/eventlog-design.md |
Documents precedence and compatibility. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+64
to
+71
| [Test] | ||
| public void Prefers_the_documented_setting_over_the_existing_spelling() | ||
| { | ||
| Environment.SetEnvironmentVariable(DocumentedVariable, "3.00:00:00"); | ||
| Environment.SetEnvironmentVariable(LegacyVariable, "5.00:00:00"); | ||
|
|
||
| Assert.That(CreateSettings().EventsRetentionPeriod, Is.EqualTo(TimeSpan.FromDays(3))); | ||
| } |
Comment on lines
+29
to
+36
| [Test] | ||
| public void Prefers_the_documented_setting_over_the_existing_spelling() | ||
| { | ||
| Environment.SetEnvironmentVariable(DocumentedVariable, "3.00:00:00"); | ||
| Environment.SetEnvironmentVariable(LegacyVariable, "5.00:00:00"); | ||
|
|
||
| Assert.That(new Settings().EventsRetentionPeriod, Is.EqualTo(TimeSpan.FromDays(3))); | ||
| } |
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.
The documented setting for how long event log items are kept is
ServiceControl/EventRetentionPeriod. The instance reads and validates it, and the usage report shows it asRetention.EventsHours, but neither persister applies it. Since 5.0.0 the RavenDB persister has read its ownServiceControl/EventsRetentionPeriodkey with a 14 day default, and the EF Core persisters copied that key. A customer who sets the documented key keeps 14 days of events, while configuration validation and the usage report suggest their value applies.Before 5.0 the instance passed its own value to the persister. #3682 moved the setting into the persister's configuration and spelled it with an extra
s.Change
PersistenceConfiguration, the base class of both persister configurations, getsReadEventsRetentionPeriod. It readsEventRetentionPeriodfirst, thenEventsRetentionPeriod, then the persister's default. The RavenDB and EF Core configurations use it.TimeSpan.TryParse, as the instance already does, so a value the instance ignores today does not stop the persister from starting.Settingsalso falls back toEventsRetentionPeriodbefore the 14 day default, soRetention.EventsHoursmatches the applied retention on installs that set only the older spelling. Validation still applies only to the documented key, so an install whose older-spelling value is outside 1 hour to 200 days still starts.docs/eventlog-design.mdnames the documented key.docs/usage-report.md, which describes this mismatch underRetention.EventsHours. That note needs updating once this merges.Upgrade impact
A customer who set
EventRetentionPeriodgets that retention after upgrading. On RavenDB, an item's expiry is set when the item is written, so the new period applies to new events only. Installs that setEventsRetentionPeriodkeep their value. This needs a release note.Testing
EventsRetentionConfigurationTests(EF Core, runs on SQL Server and PostgreSQL),RavenEventsRetentionConfigurationTests, andEventRetentionPeriodSettingsTestsfor the instance. Against the old code, the documented-key tests fail on both persisters and the older-spelling test fails for the instance.ServiceControl.UnitTests: 453/453.ServiceControl.Persistence.Tests.SqlServer655/655 andServiceControl.Persistence.Tests.PostgreSql643/643.ServiceControl.Persistence.Tests.RavenDB: 353/355. The two failures happen in SetUp with "Connection refused" from the shared embedded server, hit different tests on each run, pass when run alone, and fail the same way on master.