Skip to content
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

objstore: Add uploaded TSDB bytes metric #66

Merged
merged 32 commits into from
Sep 8, 2023

Conversation

ritaCanavarro
Copy link
Contributor

@ritaCanavarro ritaCanavarro commented Jul 21, 2023

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

Changes

As suggested on thanos-io/thanos#6544 (review) the metric regarding the number of bytes the shipper has uploaded is best suited to be on the objstore. Therefore, this PR aims to add it to the objstore.go class.

Verification

I updated the TestTimingTracingReader test in objstore_test.go but I can add more tests if needed.

Thanks!

@ritaCanavarro ritaCanavarro changed the title WIP: objstore: Add uploaded TSDB bytes metric objstore: Add uploaded TSDB bytes metric Jul 24, 2023
@bwplotka bwplotka requested a review from fpetkovski July 24, 2023 17:15
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Epic! Good start, some suggestions for the start (:

Approved CI, will try to fix setting so it allows you to use CI without approvals

CHANGELOG.md Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
@ritaCanavarro ritaCanavarro changed the title objstore: Add uploaded TSDB bytes metric WIP:objstore: Add uploaded TSDB bytes metric Jul 25, 2023
@ritaCanavarro ritaCanavarro changed the title WIP:objstore: Add uploaded TSDB bytes metric objstore: Add uploaded TSDB bytes metric Jul 25, 2023
@simonswine
Copy link
Contributor

I was separately working in exposing the fetched bytes as a histogram. Wonder if it makes sense to integrate both of our work into a single histogram: #69

@ritaCanavarro
Copy link
Contributor Author

I was separately working in exposing the fetched bytes as a histogram. Wonder if it makes sense to integrate both of our work into a single histogram: #69

Hi @simonswine, if I understood correctly, it seems that we are working on different subjects so perhaps we should keep our PRs separated.

@bwplotka
Copy link
Member

Merged your PR @simonswine.

I think @simonswine has a good point. We have two options:

A. Since we have both total fetched and distribution of fetched bytes per op, we could have similar for written bytes (total and later histogram in separate PR)
B. Although histogram's _sum might gives the same info as total counter, so we could skip adding upload bytes and focus in this PR to only add histogram (this should be easy @ritaCanavarro - just change type to histogram and use "Observe" instead of increments).

I don't have strong opinion, I trust @simonswine you have good judgment here. Would you like to help @ritaCanavarro to get this contribution to mergable state? 🤗

@bwplotka
Copy link
Member

Also, @ritaCanavarro you need to rebase now (:

@simonswine
Copy link
Contributor

I am also okay with both options.

And of course happy to help out @ritaCanavarro with this contribution.

@ritaCanavarro ritaCanavarro force-pushed the addUploadedTSDBblocksmetric branch 2 times, most recently from bc6cbbb to 3e7f602 Compare July 28, 2023 08:56
@ritaCanavarro
Copy link
Contributor Author

@bwplotka will work on the changes alongside @simonswine (thanks for the help!).

README.md Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
@ritaCanavarro ritaCanavarro changed the title objstore: Add uploaded TSDB bytes metric WIP: objstore: Add uploaded TSDB bytes metric Aug 7, 2023
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thank you @ritaCanavarro for putting the hard work in. I think we have it working, just a few more clean-ups as per my other comments

CHANGELOG.md Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore_test.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

@ritaCanavarro thanks for looking into this, I another pass and I noticed a few little things, but I think it is time to get more eyes on this

objstore_test.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ritaCanavarro ritaCanavarro changed the title WIP: objstore: Add uploaded TSDB bytes metric objstore: Add uploaded TSDB bytes metric Aug 9, 2023
objstore.go Show resolved Hide resolved
objstore.go Show resolved Hide resolved
objstore.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Looks good from my side, thank you 🎖️

rita.canavarro and others added 21 commits September 7, 2023 11:23
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Co-authored-by: Christian Simon <[email protected]>
Signed-off-by: Rita Canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
Signed-off-by: rita.canavarro <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
objstore.go Show resolved Hide resolved
Signed-off-by: rita.canavarro <[email protected]>
Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

@simonswine would you like to take another look before we merge this one?

Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Yes I think this LGTM 🚀

@fpetkovski fpetkovski merged commit 8d397d4 into thanos-io:main Sep 8, 2023
7 checks passed
@ritaCanavarro ritaCanavarro deleted the addUploadedTSDBblocksmetric branch September 8, 2023 09:43
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.

4 participants