Reorganize object store metrics #5821

rdettai · 2025-06-26T15:14:08Z

Description

Closes #5799

Rationalize object store metrics. Be more exhaustive in request and error recording.

Important: This is a breaking change as some metrics are renamed.

How was this PR tested?

TODO: add tests to the metrics wrappers

quickwit/quickwit-storage/src/metrics.rs

quickwit/quickwit-storage/src/object_storage/metrics_wrappers.rs

quickwit/quickwit-storage/src/metrics.rs

quickwit/quickwit-storage/src/object_storage/azure_blob_storage.rs

quickwit/quickwit-storage/src/metrics_wrappers.rs

rdettai · 2025-07-15T14:26:50Z

quickwit/quickwit-storage/src/metrics_wrappers.rs

+            ActionLabel::DeleteObject => "delete_object",
+            ActionLabel::DeleteObjects => "delete_objects",
+            ActionLabel::GetObject => "get_object",
+            ActionLabel::HeadObject => "head_object",


Azure: get_properties
S3: head_object
Opendal: stat

rdettai · 2025-07-15T15:15:00Z

quickwit/quickwit-storage/src/metrics_wrappers.rs

+/// This is a fork of `tokio::io::copy_buf` that enables tracking the number of
+/// bytes transferred. This estimate should be accurate as long as the network
+/// is the bottleneck.
+mod copy_buf {
+
+    use std::future::Future;
+    use std::io;
+    use std::pin::Pin;
+    use std::task::{Context, Poll, ready};
+
+    use tokio::io::{AsyncBufRead, AsyncWrite};
+
+    #[derive(Debug)]
+    #[must_use = "futures do nothing unless you `.await` or poll them"]
+    pub struct CopyBuf<'a, R: ?Sized, W: ?Sized> {
+        pub reader: &'a mut R,
+        pub writer: &'a mut W,
+        pub amt: u64,
+    }
+
+    impl<R, W> Future for CopyBuf<'_, R, W>
+    where
+        R: AsyncBufRead + Unpin + ?Sized,
+        W: AsyncWrite + Unpin + ?Sized,
+    {
+        type Output = io::Result<u64>;
+
+        fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
+            loop {
+                let me = &mut *self;
+                let buffer = ready!(Pin::new(&mut *me.reader).poll_fill_buf(cx))?;
+                if buffer.is_empty() {
+                    ready!(Pin::new(&mut self.writer).poll_flush(cx))?;
+                    return Poll::Ready(Ok(self.amt));
+                }
+
+                let i = ready!(Pin::new(&mut *me.writer).poll_write(cx, buffer))?;
+                if i == 0 {
+                    return Poll::Ready(Err(std::io::ErrorKind::WriteZero.into()));
+                }
+                self.amt += i as u64;
+                Pin::new(&mut *self.reader).consume(i);
+            }
+        }
+    }
+}


It was actually pretty simple to have an estimate for the number of bytes already downloaded when the error occurred, so I added it.

fulmicoton-dd · 2025-12-08T14:49:44Z

quickwit/quickwit-storage/src/metrics.rs

-            object_storage_get_errors_total: new_counter_vec::<1>(
-                "object_storage_get_errors_total",
-                "Number of GetObject errors.",
+            object_storage_request_duration: new_histogram_vec(


if it is ttfb it should be not be called duration!?

(technically I don't think this is ttfb btw :-/ )

Yes, I agree the terminology TTFB is a bit stretched here as it also applies to other types of queries (deletes). And it's probably not a good idea to call it request duration for GET requests when it doesn't measure the whole download duration.

I still think it's valuable to measure the time it takes for get requests to get to the byte stream. It's valuable when trying to configure the StorageTimeoutPolicy, and it's also a way to measure the read performance of the object store without having to normalize by the downloaded size. Would you be fine with calling that separate metric object_storage_get_ttfb or do you still think it's not appropriate?

@rdettai what are we measuring exactly?

@fmassot The storage SDK returning a ByteStream.
We can hope most of the meta data layer stuff is done at this point.

@rdettai-sk

it's also a way to measure the read performance of the object store without having to normalize by the downloaded size

My comment had two parts.

the first one is: you called the metric duration. This needs to be renamed.

the ttfb part it does not matter much and I agree your metric is useful. I just don't want people to start comparing hyperscalers and drawing wrong conclusions due to minor implementation differences. I would edit the comment to ttfb-like for instance.

I think we need to rename that metric and keep it...
And then, count + duration total + num bytes total.

With that, at least we have enough to compute a model
TTFB + bytes * throughput.

To summarise the changes on existing metrics:

I'll add a new metric called object_storage_get_slice_latency -> help: "TTFB-like latency of the object store get requests in seconds. This doesn't include the time to download the full response payload."

object_storage_request_duration is used only for other object store requests (only delete for now) with an updated description -> help: "Duration in seconds of the object store request"

I can add a metric to measure the total duration (download included). Should it be a counter or a histogram?

A histogram would be hard to leverage because you loose the connection to the actual download size. If we want the download throughput distribution, I think a histogram of the download throughput (wrapping the download itself with a timer) would be easier to use.

If we opt for a counter of the total duration, how would you use it to estimate the throughput?

We have retries when receiving an HTTP error response (e.g when receiving 427 slow down). I think failed attempts should not be included in the duration.

rdettai self-assigned this Jun 26, 2025

rdettai requested a review from guilload June 26, 2025 15:14

rdettai force-pushed the revamp-storage-metrics branch 2 times, most recently from 8bdb431 to 88528bf Compare July 2, 2025 11:55