Skip to content

feat(service check): add fail early option - #149

Merged
achoimet merged 1 commit into
mainfrom
feat/check-fail-early-option
Jul 9, 2026
Merged

feat(service check): add fail early option#149
achoimet merged 1 commit into
mainfrom
feat/check-fail-early-option

Conversation

@achoimet

@achoimet achoimet commented Jul 8, 2026

Copy link
Copy Markdown
Member

Problem

The ending time of our checks isn't consistently applied, which makes experiments hard to time. This adds the per-check "fail early / fail at end" option, following the reference implementation in extension-datadog.

Change

Adds a failEarly boolean to the StackState service status check:

  • Enabled (default) — in All the time mode the check fails as soon as a deviating status is observed (today's behavior).
  • Disabled — keeps collecting events for the whole step duration and reports the first-seen deviation at the end, using a past-tense message ("had status") since the status may have recovered by then.

Only affects All the time; At least once is unchanged. Parameter lives in the Advanced section, defaults to true in both the description and Prepare, so existing experiments are non-breaking.

Tests

  • Set FailEarly: true in the shared serviceCheckState test helper (matches the production default; the existing All the time failure test asserts fast-fail).
  • Added a status allTheTime fail at end subtest: deviation is deferred before the timeout and reported with the past-tense message once the step ends.
  • Full extservice suite passes, go vet clean.

Add a 'Fail early' boolean parameter to the service status check.

- When enabled (default, preserving the previous behavior), the 'All the
  time' mode fails as soon as a deviating status is observed.
- When disabled, the check keeps collecting events for the whole step
  duration and only fails at the end (with a past-tense message).

The parameter lives in the advanced section, defaults to true both in
the description and in Prepare, so existing experiments are unaffected.
Only affects 'All the time' mode; 'At least once' is unchanged.
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @achoimet's task in 1m 45s —— View job


Review complete

Tasks

  • Check previously unresolved review threads for fixes — none exist on this PR
  • Gather context (read diff, related files)
  • Review new changes (correctness, security, concurrency, perf)
  • Post findings

Findings

I reviewed extservice/service_check.go and the associated test in detail. The implementation is sound:

  • The All the time branch correctly separates the two behaviors: when FailEarly is true it fails immediately with a present-tense message (unchanged behavior); when false it latches the first deviation into state.DeviationTitle and only turns that into a checkError once completed is true, using the past-tense message.
  • The ordering of the two if blocks (record deviation, then check completed) correctly handles the edge case where the deviation is first observed on the very last poll before the step ends — it still gets reported.
  • At least once mode is untouched, matching the stated scope.
  • Default wiring in Prepare (state.FailEarly = true unless explicitly overridden) preserves backward compatibility for existing experiments that don't set the new parameter.
  • No concurrency concerns — state is per-invocation/single-threaded via the SDK's status polling, no shared mutable state introduced.
  • No security implications (no new external input handling beyond the existing boolean config parsing via extutil.ToBool).

No bugs, security issues, or race conditions found. Nothing to flag beyond what's already covered by the PR's own tests.

Note: I was unable to run go vet/go test in this sandboxed environment (commands required approval that wasn't available). The review above is based on static code analysis of the diff; the PR description states these checks pass locally.

@sonarqubecloud

sonarqubecloud Bot commented Jul 8, 2026

Copy link
Copy Markdown

@achoimet
achoimet merged commit 1da17e1 into main Jul 9, 2026
14 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant