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

feat: Decouple creating bucket metrics from instrumenting the bucket #130

Merged
merged 4 commits into from
Aug 18, 2024

Conversation

kavirajk
Copy link
Contributor

@kavirajk kavirajk commented Jul 29, 2024

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

Changes

Currently if a single process uses multiple object client, we get metrics duplicate registration failures.

With this PR,

  1. New BucketMetrics API return just a Metrics that can be created once and used for multiple clients.
  2. New Wrap API that takes metrics and returns instrumented bucket just like WrapWithMetrics API.

This PR doesn't change any existing API WrapWithMetrics

Verification

Not breaking any existing API(s). All existing tests passes

Currently if a single process uses multiple object client, we get metrics duplicate
registration failures.

With this PR,
1. New `BucketMetrics` API return just a `Metrics` that can be created once
and used for multiple clients.
2. New `Wrap` API that takes `metrics` and returns instrumented bucket just like
`WrapWithMetrics` API.

This PR doesn't change any existing API `WrapWithMetrics`

Signed-off-by: Kaviraj Kanagaraj <[email protected]>
@kavirajk kavirajk changed the title feat: Decouple metrics from bucket feat: Decouple creating bucket metrics from instrumenting the bucket Jul 29, 2024
Signed-off-by: Kaviraj Kanagaraj <[email protected]>
@kavirajk kavirajk marked this pull request as ready for review July 29, 2024 12:45
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@kavirajk
Copy link
Contributor Author

@yeya24 thanks. Can you merge it as well?

@yeya24 yeya24 merged commit 0363dad into thanos-io:main Aug 18, 2024
7 checks passed
@yeya24
Copy link
Contributor

yeya24 commented Aug 18, 2024

Done! I was hoping to get an approval from another maintainer but I think the change should be straightforward

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