[adapters] Report transaction commit progress through global_stats.#5688
[adapters] Report transaction commit progress through global_stats.#5688
Conversation
7f910ce to
0f0e2c3
Compare
0f0e2c3 to
c99275f
Compare
|
@blp any way we can export this as a prometheus metric? |
ryzhyk
left a comment
There was a problem hiding this comment.
Looks great, thanks! I would prefer to still keep logging for now. Until we have UI support, it gives the user some visual feedback on what's going on.
If we do this, only the number of not-yet-processed operators should be sufficient to expose as a metric. |
mythical-fred
left a comment
There was a problem hiding this comment.
Nit: commit subject has a trailing period — Feldera convention omits it.
crates/adapters/src/controller.rs
Outdated
| ) | ||
| } | ||
| Err(e) => error!("Error retrieving transaction commit progress: {e}"), | ||
| match self.circuit.commit_progress() { |
There was a problem hiding this comment.
Performance concern: commit_progress() on DBSPHandle calls broadcast_command, which sends a message to every worker thread and blocks until all responses arrive — a full inter-thread round-trip for every worker. Previously this was gated behind COMMIT_UPDATE_INTERVAL (≥ 10 s between calls). Now it runs on every circuit step while Committing.
During a large commit with many steps per second this doubles the inter-thread coordination overhead per step (one round-trip for step(), then another for commit_progress()). Could we rate-limit this — e.g. sample every N ms or every N steps — so the API value stays fresh but the hot path only pays the broadcast cost occasionally?
There was a problem hiding this comment.
Fred has a point, and more importantly measuring commit progress isn't free, so it does make sense to rate limit.
There was a problem hiding this comment.
I figured commit steps were expensive anyway but shrug
There was a problem hiding this comment.
They shouldn't be. We make an effort to split commit into small steps.
c99275f to
36161bc
Compare
|
OK, I added the logging back, throttled the API progress updates to once a second, and added Prometheus metrics. |
Until now, transaction commit progress has been reported through the pipeline's log, which makes it unavailable in any reasonable way through the API, to the web console, and so on. This commit adds transaction commit progress to global stats and to Prometheus metrics. Issue: #5245 Signed-off-by: Ben Pfaff <blp@feldera.com>
36161bc to
f7d7412
Compare
Until now, transaction commit progress has been reported through the pipeline's log, which makes it unavailable in any reasonable way through the API, to the web console, and so on. This commit adds transaction commit progress to global stats.
This removes the progress updates from the log. These could be added back in if it's still desirable.
Issue: #5245
Describe Manual Test Plan
I tested this by watching the metrics with a command like
watch -n.1 'curl -sk https://localhost:8888/stats|jq .global_metrics'while transactions were progressing.