-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update prom metrics to support ENG-51 renaming #305
Update prom metrics to support ENG-51 renaming #305
Conversation
All metrics are now averages, except explicit "total" metrics main_pod_average metric is removed Requires updates to template to ensure matching metric names dev2.3 rc live-traffic template being updated to match
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.
ENG-51 requirements are addressed, 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.
Internal consistency achieved
Hold up -- why would you want to average this? Instead of summing the metrics to produce the true request rate you are now averaging it. The value you are producing is no longer the request rate -- the only way to get that value is to sum up all of the instances to aggregate them. These changes make values that were being reported accurately and semantically aligned with the metric name into one that is not. If you need averages why not leave the average metrics in place? Why is this in any way necessary and a good idea? |
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.
I don't understand this. Explain further but not merging atm
changes were made to normalize metrics being returned to address ENG-51.
rate for main and tuning need to be of the same order, as that's how
they're used/being compared. I can change the names, but the intent is the
same.
Robert
…On Mon, Aug 9, 2021 at 9:47 AM Blake Watters ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I don't understand this. Explain further but not merging atm
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#305 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHDOZV7FY6ALF4GPMJ2U63T4ABCZANCNFSM5BJAD2MA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
All metrics are now averages, except explicit "total" metrics
main_pod_average metric is removed
Requires updates to template to ensure matching metric names
dev2.3 rc live-traffic template being updated to match