Currently running optimizers in Metrics#7316
Conversation
098ce61 to
4c73996
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/common/metrics.rs (2)
178-189: Type clarity: make the accumulator explicitly usizeAvoid relying on numeric literal inference; make the intent unambiguous.
- let mut total_optimizations_running = 0; + let mut total_optimizations_running: usize = 0;
191-196: Prometheus naming: avoid “total” in a Gauge name“_total” is reserved for monotonically increasing counters. This metric is a gauge, so prefer a name like optimizer_processes_running and adjust the help accordingly.
- metrics.push(metric_family( - "optimizer_total_processes_running", - "number of optimization processes running in total", + metrics.push(metric_family( + "optimizer_processes_running", + "current number of optimization processes running", MetricType::GAUGE, vec![gauge(total_optimizations_running as f64, &[])], ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/collection/src/shards/local_shard/telemetry.rs(1 hunks)lib/collection/src/telemetry.rs(2 hunks)lib/common/common/src/types.rs(2 hunks)src/actix/api/service_api.rs(2 hunks)src/common/metrics.rs(1 hunks)src/common/telemetry_reporting.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/*.rs: Prefer explicit SomeType::from(x) over implicit x.into() in Rust code
Do not use transmute_from_u8, transmute_to_u8, transmute_from_u8_to_slice, transmute_from_u8_to_mut_slice, transmute_to_u8_slice in new code; use bytemuck or zerocopy instead
Files:
lib/collection/src/telemetry.rslib/common/common/src/types.rssrc/common/metrics.rssrc/actix/api/service_api.rslib/collection/src/shards/local_shard/telemetry.rssrc/common/telemetry_reporting.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/review-rules.md)
**/src/**/*.rs: Prefer exhaustive match arms over a catch-all _ arm to avoid missing new enum variants (except in tests/benchmarks or when provably safe)
Prefer explicit field ignoring with : _ over .. in struct patterns (except in tests/benchmarks or when provably safe)
Files:
lib/collection/src/telemetry.rslib/common/common/src/types.rssrc/common/metrics.rssrc/actix/api/service_api.rslib/collection/src/shards/local_shard/telemetry.rssrc/common/telemetry_reporting.rs
🧬 Code graph analysis (1)
src/common/metrics.rs (1)
src/actix/api/service_api.rs (1)
metrics(68-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: rust-tests (windows-latest)
- GitHub Check: rust-tests (ubuntu-latest)
- GitHub Check: rust-tests (macos-latest)
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: e2e-tests
- GitHub Check: storage-compat-test
- GitHub Check: rust-tests-no-rocksdb (ubuntu-latest)
- GitHub Check: test-consistency
- GitHub Check: integration-tests
- GitHub Check: lint
- GitHub Check: integration-tests-consensus
- GitHub Check: test-consensus-compose
🔇 Additional comments (5)
lib/collection/src/shards/local_shard/telemetry.rs (1)
73-74: LGTM: gated optimizer logs via either Level4 or optimizer_logs flagThis enables metrics collection without bumping overall detail level. Nice.
src/actix/api/service_api.rs (2)
46-50: Telemetry endpoint: explicit optimizer_logs=falseClear and consistent with privacy posture for standard telemetry.
82-86: Metrics endpoint: enable optimizer logs for aggregationCorrectly sets optimizer_logs=true at Level3 to support the new metric.
src/common/telemetry_reporting.rs (1)
12-16: Reporter DETAIL updated correctlyKeeping optimizer_logs=false here is appropriate.
lib/common/common/src/types.rs (1)
36-36: All TelemetryDetail initializers include optimizer_logs Verified constant DETAIL, Default impl, and all struct literals explicitly set optimizer_logs.
1ea9600 to
3977b28
Compare
| /// Amount of optimizers currently running. | ||
| /// | ||
| /// Note: A `DetailsLevel` of 4 or setting `telemetry_detail.optimizer_logs` to true is required. | ||
| /// Otherwise, this function will return 0, which may not be correct. | ||
| pub fn count_optimizers_running(&self) -> usize { | ||
| self.shards | ||
| .iter() | ||
| .flatten() | ||
| .filter_map(|replica_set| replica_set.local.as_ref()) | ||
| .flat_map(|local_shard| local_shard.optimizations.log.iter().flatten()) | ||
| .filter(|log| log.status == TrackerStatus::Optimizing) | ||
| .count() | ||
| } |
There was a problem hiding this comment.
When reading the PR description I thought this could be done with counting with some atomic number and a guard.
But I think your approach is better, where we use state that we already have. 👍
df33643 to
c4bfa71
Compare
* Currently running optimizer count in metrics * Clearly state the prerequisites of count_optimizers_running() * Minor improvements * improve metric naming
Supersedes #7275
Implements the
optimizer_running_processesmetrics as discussed: