[ENH](fn-consumer): Add concurrency gauge - #7670
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
76878fc to
b43b83f
Compare
| opentelemetry::global::meter("chroma_fn_consumer") | ||
| .u64_observable_gauge("fn_consumer_current_compactions") | ||
| .with_description("Number of compaction jobs currently running in fn-consumer") | ||
| .with_callback(move |observer| { | ||
| observer.observe(observed_count.load(Ordering::Relaxed), &[]); | ||
| }) | ||
| .build(); |
There was a problem hiding this comment.
The observable gauge instrument returned by .build() is being dropped immediately, which will likely cause the callback to stop being invoked and the metric to not be reported.
OpenTelemetry observable instruments typically need to be kept alive for their callbacks to continue working. The return value should be stored in the FnConsumerMetrics struct:
struct FnConsumerMetrics {
current_compactions: Arc<AtomicU64>,
_gauge: ObservableGauge<u64>, // Keep instrument alive
}
impl Default for FnConsumerMetrics {
fn default() -> Self {
let current_compactions = Arc::new(AtomicU64::new(0));
let observed_count = current_compactions.clone();
let gauge = opentelemetry::global::meter("chroma_fn_consumer")
.u64_observable_gauge("fn_consumer_current_compactions")
.with_description("Number of compaction jobs currently running in fn-consumer")
.with_callback(move |observer| {
observer.observe(observed_count.load(Ordering::Relaxed), &[]);
})
.build();
Self {
current_compactions,
_gauge: gauge,
}
}
}Without storing the instrument, the metric collection will not work in production.
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
I checked the OpenTelemetry Rust 0.27 implementation used by this repo. During .build(), the SDK passes the callback to Pipelines::register_callback, which stores it as an Arc in PipelineInner.callbacks. The returned ObservableGauge is only a PhantomData marker and has no Drop behavior, so dropping that value does not unregister the callback or stop collection. The pipeline continues to own the callback and the captured atomic value, so this metric remains live.
| // in-progress slot until that completion is drained. Therefore, pending | ||
| // completions are bounded by max_concurrent_workers and need no backpressure. | ||
| let (completion_tx, completion_rx) = mpsc::unbounded_channel::<FnDispatchCompletion>(); | ||
| let metrics = FnConsumerMetrics::default(); |
There was a problem hiding this comment.
I believe that dropping this may lose the metrics. Otherwise aI don't get the reason for the clone, either. It reads funny.
There was a problem hiding this comment.
The extra clone is indeed unnecessary and reads oddly: the original metrics value is dropped immediately, so it could be moved directly into the spawned task. That is a harmless readability issue rather than a correctness issue. I also checked the gauge lifetime against OpenTelemetry Rust 0.27: .build() registers the callback into the SDK pipeline, which owns it independently; the returned ObservableGauge is a marker with no Drop behavior, so dropping it does not lose the metric.
Summary
fn_consumer_current_compactionsobservable gaugeTesting