-
Notifications
You must be signed in to change notification settings - Fork 530
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
Consolidate backend metrics and add hedged request metric #790
Consolidate backend metrics and add hedged request metric #790
Conversation
Thanks for digging into this. I was thinking more like consolidating the three instrumentation.go files we have in gcs/s3/azure and then rolling this new metric into that. It's a shame that these metrics names are different but we should keep the metrics backwards compatible: All new metrics should not contain the name of the backend and should be the same no matter what backend you choose. |
I see, thanks for the clarification. So to reiterate: capture duration metrics in a consolidated instrumentation.go (using the current instrumented transport approach) and grab the hedged specific metrics via |
Yup, that's what I was thinking. |
e89b478
to
1d6d7ed
Compare
Hi @joe-elliott, I've been working on this over the past few evenings and feel it's getting to a point where it'd be valuable to get some feedback on the approach. When you have moment would you mind having a quick look? Thanks |
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.
A well thought out addition. Thank you.
Please add a changelog entry mentioning the new metrics and the deprecation of the old. Other than that this look ready to go.
a834cc5
to
dc06bd0
Compare
@joe-elliott This PR is good to go if you're happy with it. |
873f441
to
4a1572f
Compare
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.
Excellent! Thank you for the contribution.
@joe-elliott No problem! I'm pretty new to Go so this has been a great learning opportunity on a reasonably large, real-world Go product. I'll look at picking up another issue now that this has been merged. |
What this PR does:
This PR does two things:
Which issue(s) this PR fixes:
Fixes #760
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]