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.

Sign up for GitHub

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

Jump to bottom

Change query function to rate for etcd_network_client_grpc_sent_bytes_total metric #1185

Merged
merged 1 commit into from
Apr 24, 2020
Merged

Change query function to rate for etcd_network_client_grpc_sent_bytes_total metric #1185

merged 1 commit into from
Apr 24, 2020

Conversation

tosi3k
Copy link
Member

@tosi3k tosi3k commented Apr 22, 2020

rate query function better reflects the change in time in case of etcd_network_client_grpc_sent_bytes_total metric than irate.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 22, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @tosi3k. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 22, 2020
@k8s-ci-robot k8s-ci-robot requested review from mborsz and oxddr April 22, 2020 09:44
oxddr
oxddr reviewed Apr 22, 2020
View reviewed changes
Copy link
Contributor

@oxddr oxddr left a comment

Choose a reason for hiding this comment

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

/ok-to-test

clusterloader2/pkg/prometheus/manifests/dashboards/master-dashboard.dashboard.py
@@ -103,7 +103,7 @@ def api_call_latency(title, verb, scope, threshold):
d.simple_graph("etcd leader", "etcd_server_is_leader", legend="{{instance}}"),
d.simple_graph(
"etcd bytes sent",
"irate(etcd_network_client_grpc_sent_bytes_total[1m])",
"rate(etcd_network_client_grpc_sent_bytes_total[1m])",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you provide an explanation why rate is prefered over irate in this case? I suspect the reason is not obvious. Let's save next person time on figuring it out :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Experience shows that irate doesn't work good with this metric. When I conducted A/B testing between two different 5k runs here, graphs of the aforementioned metric looked completely different with irate applied. With rate function the graphs looked pretty much the same.

From Prometheus' doc:
irate should only be used when graphing volatile, fast-moving counters. Use rate for alerts and slow-moving counters, as brief changes in the rate can reset the FOR clause and graphs consisting entirely of rare spikes are hard to read.

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 22, 2020
@oxddr
Copy link
Contributor

oxddr commented Apr 24, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oxddr, tosi3k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 6a9fefa into kubernetes:master Apr 24, 2020
@tosi3k tosi3k deleted the prometheus-etcd-rate branch February 9, 2021 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxddr oxddr oxddr left review comments

@mborsz mborsz Awaiting requested review from mborsz

Assignees

@oxddr oxddr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tosi3k @k8s-ci-robot @oxddr