Skip to content

Reorganize object store metrics #5821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reorganize object store metrics #5821

wants to merge 4 commits into from

Conversation

rdettai
Copy link
Collaborator

@rdettai rdettai commented Jun 26, 2025

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

@rdettai rdettai self-assigned this Jun 26, 2025
@rdettai rdettai requested a review from guilload June 26, 2025 15:14
@rdettai rdettai force-pushed the revamp-storage-metrics branch from 68c7373 to 8bdb431 Compare July 2, 2025 10:08
@rdettai rdettai force-pushed the revamp-storage-metrics branch from 8bdb431 to 88528bf Compare July 2, 2025 11:55
@rdettai rdettai force-pushed the revamp-storage-metrics branch from 0d1674d to 93ce4eb Compare July 15, 2025 14:24
ActionLabel::DeleteObject => "delete_object",
ActionLabel::DeleteObjects => "delete_objects",
ActionLabel::GetObject => "get_object",
ActionLabel::HeadObject => "head_object",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Azure: get_properties
S3: head_object
Opendal: stat

- Use explicit label values
- Track download at the copy level

Unified label values for object store actions.
@rdettai rdettai force-pushed the revamp-storage-metrics branch from 93ce4eb to cb7eb48 Compare July 15, 2025 15:00
Comment on lines +334 to +379
/// 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);
}
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rdettai rdettai requested a review from fulmicoton-dd July 15, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix the object_storage_get_errors_total metric
2 participants