Skip to content

Jitter DecisionLogPlugin upload interval between min/max delay - #158

Open
arimu1 wants to merge 1 commit into
open-policy-agent:mainfrom
arimu1:fix/78-decision-log-jitter
Open

Jitter DecisionLogPlugin upload interval between min/max delay#158
arimu1 wants to merge 1 commit into
open-policy-agent:mainfrom
arimu1:fix/78-decision-log-jitter

Conversation

@arimu1

@arimu1 arimu1 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Problem

DecisionLogPlugin.start() scheduled decision-log flushes via scheduleAtFixedRate(...) using only min_delay_seconds as both the initial delay and the fixed period. Every instance in a fleet therefore uploads on the same cadence, and max_delay_seconds (already present on Config.DecisionLogsConfig, with a default of 600s) was never propagated to the scheduling logic at all. OPA Go jitters the decision log reporting interval between min_delay_seconds and max_delay_seconds to spread load across instances and match operator expectations.

Approach

Mirrors BundleDownloader's existing chained random-delay pattern (scheduleNextPoll), which this SDK already uses for bundle and discovery polling:

  • DecisionLogPlugin.initialize() now also calls .setMaxDelaySeconds(logsConfig.getMaxDelaySeconds()) when building the DecisionLogs instance (previously only min_delay_seconds was wired through).
  • Added a maxDelaySeconds field + getter/setter to the inner DecisionLogs class, mirroring the existing minDelaySeconds field.
  • Replaced scheduler.scheduleAtFixedRate(...) with a new private scheduleNextFlush(minDelay, maxDelay) that re-schedules itself after each flush with a uniformly random delay in [minDelay, maxDelay] (ThreadLocalRandom), swallowing Exception from flush() (which already logs internally, same as downloadBundle()) so the chain keeps running, and stopping cleanly on RejectedExecutionException after shutdown — same structure as BundleDownloader.scheduleNextPoll.
  • If max_delay_seconds is unset, it now defaults to 2 * min_delay_seconds (previously it was silently ignored entirely) instead of an unrelated hardcoded constant.
  • Switched the plugin's scheduler construction to BundleDownloader.newPollScheduler("opa-decision-log-scheduler"), matching BundlePlugin and DiscoveryPlugin, so it uses daemon threads that drop pending delayed tasks on shutdown (consistent with the rest of the plugin's stop() handling).

validate() already checked min_delay_seconds <= max_delay_seconds for both the top-level config and the nested reporting config, so no changes were needed there.

Tests

Added two tests to DecisionLogPluginTest, following the timing-based style already used in DiscoveryPluginTest for its chained-scheduling tests:

  • start_periodicFlush_usesJitteredChainedSchedule — sets min == max == 1s for a deterministic interval, logs an event before each tick, and verifies at least two "Flushed %d decision log events" debug logs occur ~1s apart, proving the schedule re-chains itself rather than firing once.
  • start_onlyMinDelayConfigured_defaultsMaxToTwiceMin — sets only min_delay_seconds = 1 with max_delay_seconds explicitly null, and verifies a flush occurs within 2.5s, proving the fallback is 2 * min (2s here) rather than an unrelated default that would never fire in the test window.

Ran locally with JDK 17 / Gradle:

./gradlew :opa-services:test --tests "io.github.open_policy_agent.opa.plugins.DecisionLogPluginTest"

All 23 tests in the class pass (21 pre-existing + 2 new). Also ran the full :opa-services:test suite plus :opa-services:checkstyleMain/:opa-services:checkstyleTest — build succeeds; checkstyle produces only pre-existing warn-level MethodName warnings project-wide (same underscore-style test names used throughout the existing test suite, not introduced by this change) and no errors.

Fixes #78
Fixes #78

@arimu1
arimu1 requested a review from a team as a code owner July 19, 2026 10:59
DecisionLogPlugin.start() scheduled flushes via scheduleAtFixedRate
using only min_delay_seconds, causing every instance in a fleet to
upload on the same fixed cadence. OPA Go jitters the decision log
reporting interval between min_delay_seconds and max_delay_seconds to
spread load, and this SDK's BundleDownloader already implements that
chained random-delay pattern for bundle polling.

Wire max_delay_seconds through DecisionLogPlugin -> DecisionLogs
(previously only min_delay_seconds was propagated from config) and
replace scheduleAtFixedRate with scheduleNextFlush, mirroring
BundleDownloader.scheduleNextPoll: each flush re-schedules itself with
a uniformly random delay in [min, max], swallowing exceptions from
flush() (which already logs internally) so the chain keeps running,
and stopping cleanly on RejectedExecutionException after shutdown.
Also switch the plugin's scheduler to
BundleDownloader.newPollScheduler(...), matching BundlePlugin and
DiscoveryPlugin, for daemon threads that drop pending delayed tasks on
shutdown.

When max_delay_seconds is unset, it now defaults to 2x min_delay_seconds
instead of being silently ignored.

Fixes open-policy-agent#78
Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>
@arimu1
arimu1 force-pushed the fix/78-decision-log-jitter branch from b85fce7 to dd6ac90 Compare July 20, 2026 03:01
Comment on lines +123 to +128
int minDelaySeconds =
(decisionLogs.getMinDelaySeconds() != null) ? decisionLogs.getMinDelaySeconds() : 300;
int maxDelaySeconds =
(decisionLogs.getMaxDelaySeconds() != null)
? decisionLogs.getMaxDelaySeconds()
: minDelaySeconds * 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The min/max defaulting here can produce an effective min > max that validate() won't catch, because validate() only compares the two when both are explicitly set — and the 300 default is injected here, after validation runs.

A user sets only max_delay_seconds: 120. validate() sees min = null and skips its check. Then this code defaults minDelaySeconds to 300, giving effective bounds 300 / 120. The minDelay >= maxDelay guard in scheduleNextFlush then silently falls back to a fixed 300s delay, quietly ignoring the ceiling the operator configured.

OPA Go handles this by validating the post-default values and rejecting the config when defaulted min > max. Could we do the same here?

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.

Jittered upload interval

2 participants