Skip to content

Metrics: Fix objstore_bucket_operation_transferred_bytes never reco… - #283

Open
leizor wants to merge 2 commits into
thanos-io:mainfrom
leizor:leizor/missing-xfered-bytes-readat
Open

leizor wants to merge 2 commits into
thanos-io:mainfrom
leizor:leizor/missing-xfered-bytes-readat

Conversation

@leizor

@leizor leizor commented Sep 16, 2026 •

Copy link
Copy Markdown

…rded when the underlying reader is read via io.ReaderAt

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

timingReaderSeekerReaderAt.ReadAt now updates metrics so the objstore_bucket_operation_transferred_bytes records correctly. This fixes the metric for multipart uploads, which currently always only records 0.

Verification

Added two tests:

  • TestMetricBucket_Upload_ReadAt
  • TestTimingReader_ConcurrentReadAt

I've also run this code in a dev environment I have.

@leizor
leizor force-pushed the leizor/missing-xfered-bytes-readat branch from a62b02a to 526ce87 Compare September 16, 2026 20:40
@leizor

leizor commented Sep 16, 2026 •

Copy link
Copy Markdown
Author

That failing test should be fixed by #284.

@leizor
leizor marked this pull request as ready for review September 16, 2026 21:19
@leizor
leizor force-pushed the leizor/missing-xfered-bytes-readat branch from 526ce87 to ca4f1ec Compare September 17, 2026 23:02
@leizor
leizor marked this pull request as draft September 17, 2026 23:46
@leizor
leizor force-pushed the leizor/missing-xfered-bytes-readat branch 6 times, most recently from 61b6399 to 431a41f Compare September 21, 2026 19:58
@leizor
leizor marked this pull request as ready for review September 21, 2026 21:01
Comment thread objstore.go Outdated
readBytes: 0,
}

trc.readAtTracker = newReadAtTracker(isFailureExpected)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Won't this affect all operations, including Get and GetRange? We would be creating a mutex and a map per per bucket read which potentially is a significant performance regression.

Perhaps we can instantiate the specialized read tracker only for the Upload operation.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good idea - done in 770bfe5.

@leizor
leizor force-pushed the leizor/missing-xfered-bytes-readat branch 3 times, most recently from 2e01b37 to 770bfe5 Compare October 5, 2026 16:18
leizor added 2 commits October 8, 2026 14:06
when the underlying reader is read via io.ReaderAt

Signed-off-by: Justin Lei <lei.justin@gmail.com>
Signed-off-by: Justin Lei <lei.justin@gmail.com>
@leizor
leizor force-pushed the leizor/missing-xfered-bytes-readat branch from 770bfe5 to 143bc9f Compare October 8, 2026 21:06
@leizor

leizor commented Oct 29, 2026

Copy link
Copy Markdown
Author

@fpetkovski Are you waiting on anything from me to get this merged?

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.

2 participants