feat: add push, fetch, and sync latency metrics - #362
Conversation
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus 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 |
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P3] Link the tracking issue required by the contribution policy
CONTRIBUTING.md:55
This feature PR has no linked issue, and the active quality-signal triage request still asks for one. That leaves the expected semantics for the new operational metrics—including whether sync lag means public servability or end-to-end queue completion—without a durable, reviewable decision. Please add the issue that approved this work (or open one), state the intended metric boundaries and trust model there, and link it from the PR.
Findings
-
[P2] Sample sync lag when the mirror actually becomes visible
crates/gitlawb-node/src/sync.rs:428
upsert_mirror_repohas already registered a public, servable mirror, but the new observation is timestamped only afterreplicate_encrypted_blobsfinishes andmark_sync_donesucceeds. The blob-replication step performs additional origin/network/storage work that is unrelated to whether Git smart HTTP can serve the ref, and it can be arbitrarily slow. A healthy mirror that is immediately cloneable can therefore be reported as minutes behind, sogitlawb_sync_lag_secondsdoes not measure the advertised origin-to-visible latency.The root cause is using queue completion as a proxy for the visibility transition. Record a
visible_attimestamp immediately after the successful, admitted mirror registration and use that for this histogram; retain a separate completion metric if encrypted-blob replication and queue settlement also need an SLO. Add a test that makes the post-registration replication work slow and proves the visibility histogram is unaffected. -
[P2] Do not count quarantined mirrors as successful visibility
crates/gitlawb-node/src/sync.rs:414
In iCaptcha enforcement mode, an unavailable proof makesquarantinedtrue, but a successfulupsert_mirror_repostill setsmirror_visibleand records the lag. Quarantined mirrors are intentionally hidden from clone, fetch, and listings, so this reports successful visibility for a ref no caller can serve. This becomes especially misleading during an iCaptcha outage: the histogram will show completed low-lag mirrors while every affected repository remains unavailable.The root cause is treating successful persistence of a mirror row as equivalent to its admission for serving. Carry the actual admission/servability result through the sync success path and observe visibility only when the row is non-quarantined; a pre-existing quarantine must remain non-visible after a resync because
upsert_mirror_repodeliberately preserves it. Add an enforced-iCaptcha regression test for both a newly quarantined mirror and a resync of one already quarantined. -
[P2] Bound untrusted origin timestamps before adding them to the histogram
crates/gitlawb-node/src/sync.rs:187
The new helper accepts any parseable past timestamp. Both ingress paths persist a peer-provided value, and the default HTTP peer-write mode accepts an unsigned request claiming a known peer. A request with1970-01-01T00:00:00Zreaches the successful sync path and permanently adds a multi-decade value to the process-wide histogram's sum/+Inf bucket. Because Prometheus histograms retain their cumulative sum and bucket counts for the lifetime of the process, one request can corrupt latency alerts, averages, and quantiles until restart; repeated requests can keep the SLO unusable.The root cause is promoting a remote timestamp to trusted measurement input without a local receipt anchor or a maximum acceptable age. Preserve the origin timestamp as optional diagnostic metadata, but only observe it when it is within a documented bounded skew of a locally recorded receipt/enqueue time; otherwise increment a distinct invalid/stale-origin counter or omit the sample. This should be enforced uniformly for HTTP and gossipsub ingress, with tests for malformed, future, plausible, and arbitrarily old values.
Summary
Validation