encryption: add sidecar and KEK metrics (Stage 9C-2, §9.2) - #1223
Conversation
Adds the three remaining §9.2 metrics that have live sources today:
elastickv_encryption_active_dek_id{purpose}
elastickv_encryption_sidecar_raft_index
elastickv_encryption_kek_unwrap_seconds
StateCache gains observability-only mirrors of sidecar.Active.Raft and
sidecar.RaftAppliedIndex, refreshed by the same RefreshFromSidecar call
that maintains the decision mirrors so they cannot drift from them. A
polling observer publishes them, sampling immediately at startup rather
than making an operator wait a full interval after a restart.
Both purpose series are published at 0 on construction: without that,
an alert on active_dek_id == 0 cannot distinguish "not bootstrapped"
from "this node never reported", which are very different incidents.
KEK latency is captured by decorating the loaded KEK source once in
main.go, so every unwrap path (startup hydration, bootstrap apply,
rotation apply) is timed without per-path instrumentation. Failed
unwraps are timed too — a KMS outage surfaces as slow errors, and
excluding them would hide the signal the histogram exists to expose.
last_proposed_index_per_raft_dek still needs the §5.4 raft-DEK Wrap
path and stays unregistered rather than always-zero.
Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
@codex review |
|
@claude review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @bootjp's task in 2m 14s —— View job Review: encryption: add sidecar and KEK metrics (Stage 9C-2, §9.2)
Note: SummaryThis is a clean, well-scoped observability-only change. I traced the two things most likely to hide a bug in a change like this and both check out:
One thing worth a second look, not a blocker:
Everything else matches what the description claims:
No correctness, data-loss, or concurrency issues found. Nice, tightly-scoped PR — good adherence to the design-doc-first workflow and the five-pass self-review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16592c464f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| c.activeStorageDEKID.Store(sc.Active.Storage) | ||
| c.storageEnvelopeActive.Store(sc.StorageEnvelopeActive) | ||
| c.activeRaftDEKID.Store(sc.Active.Raft) | ||
| c.sidecarRaftAppliedIndex.Store(sc.RaftAppliedIndex) |
There was a problem hiding this comment.
When a stale or duplicate enable-storage-envelope/enable-raft-envelope entry is applied, the no-op branches in applyEnableStorageEnvelope and applyEnableRaftEnvelope advance and persist sc.RaftAppliedIndex but return without calling RefreshFromSidecar. Because the new sidecarRaftAppliedIndex mirror is the sole source for elastickv_encryption_sidecar_raft_index, the metric remains behind the actual persisted sidecar until another fresh encryption mutation or restart, potentially producing a false sidecar-divergence signal.
Useful? React with 👍 / 👎.
| // the single place the KEK source is loaded — means every unwrap | ||
| // path (startup hydration, bootstrap apply, rotation apply) is | ||
| // timed without each one needing its own instrumentation. | ||
| kekUnwrapper := monitoring.NewTimedKEKUnwrapper(kekWrapper, metricsRegistry.KEKUnwrapObserver()) |
There was a problem hiding this comment.
On every successful startup with a configured KEK, loadKEKAfterPreNonceStartupGuards invokes both CheckStartupGuards and kek.VerifyWrapper using the raw wrapper before this decorator is constructed. Their KEK unwrap round trips—potentially slow remote-KMS calls—are therefore omitted from elastickv_encryption_kek_unwrap_seconds; on a fresh node the preflight unwrap can be the only startup unwrap, leaving the histogram empty despite a completed call. Construct or inject the timed wrapper before these startup checks so the metric covers the advertised unwrap paths.
Useful? React with 👍 / 👎.
Two P2s from review. The no-op apply branches (stale DEKID, already-active cutover) advance and persist sc.RaftAppliedIndex but returned without refreshing the StateCache, so the mirror — and therefore elastickv_encryption_sidecar_raft_index — sat behind the durable sidecar until the next fresh mutation or a restart, which reads as a false sidecar-divergence signal. Fixed structurally rather than at the two named branches: all eight sidecar-write sites now go through Applier.persistSidecar, which pairs the write with the refresh. Patching only the reported branches would have left the same latent defect in every future one. The KEK decorator was constructed after loadKEKAfterPreNonceStartupGuards had already run CheckStartupGuards and kek.VerifyWrapper against the raw wrapper. Both perform real unwrap round trips, and on a fresh node the preflight unwrap can be the only one that ever happens, so elastickv_encryption_kek_unwrap_seconds could stay empty despite completed KMS calls. The source is now decorated inside the loader, before those guards run. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE
|
Both P2s fixed in No-op sidecar writes left the mirror behind. Confirmed: the stale-DEKID and already-active branches call Fixed structurally, not at the two named branches. I audited all KEK decorated too late. Confirmed: Revert-checked (restore byte-exact): reverting the two no-op branches to bare
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What
Stage 9C-2 of
docs/design/2026_04_29_partial_data_at_rest_encryption.md— the three remaining §9.2 metrics that have live sources today.elastickv_encryption_active_dek_id{purpose}StateCachemirror ofsidecar.Active.{Storage,Raft}elastickv_encryption_sidecar_raft_indexStateCachemirror ofsidecar.RaftAppliedIndexelastickv_encryption_kek_unwrap_secondsAfter this, only
last_proposed_index_per_raft_dekremains from §9.2 — it needs the §5.4 raft-DEK Wrap path, so it stays unregistered rather than always-zero.Decisions worth reviewing
Both purpose series are published at 0 on construction. Without that, an alert on
active_dek_id == 0cannot distinguish "not bootstrapped" from "this node never reported" — very different incidents. Pinned by a revert-checked test.The mirrors are refreshed by the existing
RefreshFromSidecar, the same call that maintains the decision mirrors, so metrics state cannot drift from decision state. They are observability-only and explicitly documented as such — the raft envelope path resolves its own key id through the raft envelope runtime, not through this mirror.KEK timing decorates once, at the single load site. Every unwrap path — startup hydration, bootstrap apply, rotation apply — is timed without each needing its own instrumentation.
Failed unwraps are timed. A KMS outage shows up as slow errors; excluding them would hide exactly the signal the histogram exists for.
The decorator wraps errors rather than carrying a
//nolint.wrapcheckflagged the pass-through;errors.WrapfpreservesIs/As, so the startup guards that matchErrKEKMismatchstill see through it — pinned by a test that assertserrors.Issurvives the decorator.A test that was lying
TestTimedKEKUnwrapperTimesAndPreservesFailuresoriginally assertedCollectAndCount(kekUnwrapSecond) == 1. That counts series, and a histogram is one series whether or not anything was observed — so it passed with the observation removed. It now asserts the gathered histogram'sSampleCount, and revert-check C below fails as it should.Behavior change / risk
Observability only; no decision reads these values. The one non-metrics change is the KEK decorator sitting on the unwrap path: it adds two
time.Now()calls and now wraps the returned error with the source name.errors.Is/Asare preserved, verified by test.NewTimedKEKUnwrapperreturns the inner source unchanged when metrics are absent, so an unmetered node keeps the exact object it had before.Test evidence
go test . ./monitoring/ ./internal/encryption/... ./store/ -race -count=1— all passgolangci-lint run(full repo) — 0 issues, no//nolintaddeddiff -q):RefreshFromSidecarstops mirroring the raft slot →TestStateCacheMirrorsRaftSlotAndSidecarIndexFAILsTestEncryptionMetricsPublishPreBootstrapPostureAtConstructionFAILsTestTimedKEKUnwrapperTimesAndPreservesFailuresFAILs (only after fixing the assertion above — the first version passed)Self-review (five passes)
atomic.Uint32/Uint64written by the same FSM-apply goroutine that already writes the decision mirrors; the observer only reads. Observer goroutine exits on ctx cancel. Race-clean across all touched packages.time.Now()calls per KEK unwrap (rare: startup and encryption applies only). The gauge collector ticks at the existingraftMetricsObserveIntervaland does no I/O — that is why the values are mirrored into the cache instead of read from the sidecar file.Sidecarvalue, so a reader cannot observe a half-updated pair.https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE