Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: 0f90d808a0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: somaz <genius5711@gmail.com>
Signed-off-by: somaz <genius5711@gmail.com>
0f90d80 to
418df1f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9860 +/- ##
==========================================
- Coverage 81.32% 81.32% -0.01%
==========================================
Files 264 264
Lines 41002 41003 +1
==========================================
Hits 33345 33345
- Misses 7656 7657 +1
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
|
Thanks for the approval. The only red check is docs-lint, and it failed on a 408 timeout while crawling latest/tasks/security/index.xml, a page this PR does not touch. The same workflow has been green on main since then, so it looks like a flaky link check. Could you rerun the failed job when you get a chance? |
| watchableDepth = metrics.NewGauge( | ||
| "watchable_depth", | ||
| "Current depth of watchable queue.", | ||
| "Number of updates coalesced into the snapshot being handled, or 0 when idle.", |
There was a problem hiding this comment.
I think we probably should avoid changing the meaning of watchable_depth here.
len(snapshot.Updates) can still be useful, but it tells us how many updates arrived together in one batch—not how many updates are currently waiting to be processed.
If we expose it as a gauge, Prometheus might scrape after the batch has finished and the value has been reset to zero. That means it can miss a large batch even though the subscriber was temporarily falling behind.
Since the existing metric is permanently zero, I'd suggest removing watchable_depth (and its dashboard/documentation entry) as a breaking cleanup rather than repurposing it.
A follow-up can add an explicitly named histogram/counter for snapshot update counts, potentially both raw(watchable_snapshot_updates) and post-coalescing(watchable_coalesced_updates) counts, if that signal is valuable. For detecting control-plane lag, a queue-wait metric like watchable_queue_wait_seconds would be a more direct measure.
There was a problem hiding this comment.
Thanks, that makes sense. Resetting to zero after each batch means what a scrape sees depends on timing, so it can miss exactly the bursts it is meant to show. I've turned this into a removal instead. One thing had to move with it: both envoy-gateway-global.json and global-ratelimit.json build their Namespace and Runner template variables from label_values(watchable_depth, ...), so removing the metric on its own would leave those selectors empty and blank every panel that filters on them. They now read from watchable_subscribe_total, which carries the same runner label, the depth panel is gone, and the helm golden files are regenerated. I've left the batch size histograms and a queue wait metric for a follow-up so this PR stays a cleanup.
…used it Signed-off-by: somaz <genius5711@gmail.com>
… fix/watchable-depth-metric Signed-off-by: somaz <genius5711@gmail.com>
somaz94
changed the title
This branch has not been deployed
3 participants
What this PR does / why we need it:
watchable_depthrecordedlen(subscription), wheresubscriptionis the channelreturned by
watchable.Map.Subscribe(). That channel is unbuffered(
downstream := make(chan Snapshot[K, V])in the watchable library), so the gaugecould only ever read 0, while the shipped Grafana dashboard graphed it as queue depth.
An earlier revision of this PR repurposed the gauge as
len(snapshot.Updates). Asdiscussed in review, that is the size of one batch rather than a backlog, and because
it returns to 0 once the batch is handled, whether a scrape sees it depends on timing.
So this PR now removes the metric as a breaking cleanup instead.
Removing it touches more than the metric definition. Both
envoy-gateway-global.jsonand
global-ratelimit.jsonbuild theirNamespaceandRunnertemplate variablesfrom
label_values(watchable_depth, ...), so dropping the metric on its own would leavethose selectors empty and blank every panel that filters on them. They now read from
watchable_subscribe_total, which carries the samerunnerlabel and is recorded forevery handled snapshot. The Depth stat panel is removed and the Statistics panel moves
into its slot.
Batch-size histograms and a queue-wait metric are left for a follow-up, so this PR
stays a cleanup.
Validation was run locally on darwin/arm64 with the go.mod-pinned Go.
make lintandmake gen-checkboth pass and leave the tree clean, andgo test ./internal/message/... ./internal/metrics/...passes.make helm-templateregeneratestest/helm/gateway-addons-helm/*.out.yaml, and the only change in them is the dashboardedit above.
Which issue(s) this PR fixes:
Fixes #9776
Item 2 of that issue (subscribe-duration bucket spacing) already landed in #9812;
this is item 1.
PR Checklist
git commit -s)./api.make lintandmake gen-checkpass locally, and the unit tests for the touched packages pass.watchable_depthrow fromsite/content/en/latest/tasks/observability/gateway-exported-metrics.md.release-notes/current/breaking_changes/9776-remove-watchable-depth.md.make gen-checkis clean.release-notes/current/breaking_changes/.