feat(metrics): export KV counters and populate the tiered-log family (#44) - #69
Merged
Conversation
…44) The last two families from #44. Both are worth keeping rather than deleting: per-namespace KV operation rates and hot/warm hit rates are the numbers an operator needs to size a hot buffer or spot a workload change. KV was recorded nowhere and, separately, never emitted — `exportPrometheus` had no KV branch at all, so the series did not exist rather than reading zero. Added `writeKVMetrics` and wired get/set/delete in the KV dispatch paths, using the per-namespace metrics `registerKVNamespace` already returned and discarded. The tiered-log family was already written by `exportPrometheus`, but `registerTieredLog` had no callers, so nothing ever created an entry to export. The shard now registers its log once at wire-up and hands the pointer to the stream handler, so `getPayloadAndTier` — which already decides hot vs warm per record — records the hit without a registry lookup on the read path. `cold_hits` stays at 0 by design: that path consults the hot ring and the warm store only, and never cold. Left visible rather than faked. ## Tests Two more value-asserting e2e tests: two sets, one get and one delete produce 2/1/1 on the KV counters; an append and read produce a non-zero hot-hit count. Both fail with the instrumentation stashed. test-unit, test-integration and the kv/stream e2e filters pass — the 4 kv/cluster failures are the pre-existing set from #62.
Found reviewing this PR's own diff. decrementKeyCount was an unguarded fetchSub, and key_count is not rebuilt on recovery — the apply path never goes through recordSet. So after a restart the counter is 0 while keys exist on disk, and deleting any recovered key wrapped the gauge to u64 max. That is the exact failure this issue is about: a number that looks real and is not. Saturating at zero via CAS, with a regression test that restarts the server, deletes a recovered key, and asserts the gauge stays sane. The test fails against the unguarded version.
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 last two families from #44. Closes it.
The decision the issue asked for
Export, not delete. Per-namespace KV operation rates and hot/warm hit rates are exactly what you need to size a hot buffer or notice a workload shift — deleting them would remove the observability, not the dead code problem.
KV — recorded nowhere, and never emitted
Two separate gaps.
recordGet/recordSet/recordDeletehad no callers, andexportPrometheushad no KV branch at all — so unlike the other families, the series didn't exist rather than reading0.Added
writeKVMetricsplus the export loop, and wired the three record calls into the KV dispatch paths using the per-namespace metrics thatregisterKVNamespacealready returned and the code discarded.Tiered log — written but never registered
The opposite shape:
writeTieredLogMetricswas already called byexportPrometheus, butregisterTieredLoghad zero callers, so no entry ever existed to iterate over.The shard now registers its log once at wire-up and hands the pointer to the stream handler.
getPayloadAndTieralready decides hot vs warm per record, so recording there costs nothing extra — and resolving the pointer at wire-up keeps a registry lookup off the read path.cold_hitsstays 0 by design — that path consults the hot ring and warm store only, never cold. Left visible rather than faked, since a fabricated number is the exact problem this issue is about.Tests
Two more value-asserting e2e tests: two sets, a get and a delete produce 2/1/1 on the KV counters; an append plus read produces a non-zero hot-hit count. Both fail with the instrumentation stashed.
test-unit,test-integration,e2e/kvande2e/streamall pass — the 4kv/clusterfailures are the pre-existing set from #62.