-
Notifications
You must be signed in to change notification settings - Fork 79
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
Metrics: Wire up objstore_bucket_operation_duration_seconds
in iter
operations
#79
Conversation
@@ -27,6 +27,7 @@ require ( | |||
golang.org/x/oauth2 v0.4.0 | |||
golang.org/x/sync v0.1.0 | |||
google.golang.org/api v0.103.0 | |||
google.golang.org/grpc v1.53.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this was forgotten or specific to my setup - happy to remove it if undesired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you are correct. This might be a miss on previous prs
…r` operations Trying to get some statistics in Mimir about the time we're spending in our `iter` operations I realised that despite these operations showing up as part of the labels their value was always 0. Soon enough and with the help of Peter, I realised that these were not wired up in `objstore.go`. This PR does exactly that and measures the duration of the whole `iter` operation. There's one big caveat though, `iter` operations take a callback as an argument which is executed per each item returned. The computed duration will include whatever time we spend on each of these callbacks which might an undesirable effect for users of the library. Sadly, I can't get around this without changing the interface for `iter` to (e.g.) return JUST the time the network operation took. In the meantime, I've done the best I've could without incurring in a big change and documented the behaviour as part of the Histogram's HELP. Co-Authored-By: Peter Štibraný <895919+pstibrany@users.noreply.github.com> Signed-off-by: gotjosh <josue.abreu@gmail.com>
Signed-off-by: gotjosh <josue.abreu@gmail.com>
Signed-off-by: gotjosh <josue.abreu@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. I want to verify the go mod change myself first before merging it
Trying to get some statistics in Mimir about the time we're spending in our
iter
operations I realised that despite these operations showing up as part of the labels their value was always 0.Soon enough and with the help of Peter, I realised that these were not wired up in
objstore.go
.This PR does exactly that and measures the duration of the whole
iter
operation. There's one big caveat though,iter
operations take a callback as an argument which is executed per each item returned. The computed duration will include whatever time we spend on each of these callbacks which might an undesirable effect for users of the library.Sadly, I can't get around this without changing the interface for
iter
to (e.g.) return JUST the time the network operation took. In the meantime, I've done the best I've could without incurring in a big change and documented the behaviour as part of the Histogram's HELP.Verification
I've used a test which I then deleted because the histogram is based on time and we don't seem to mock time so it's not worth committing.