Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

Uses mean instead of sum in Grafana queries to avoid plotting variations at different timescale #1961

Merged
merged 2 commits into from Jul 17, 2018

Conversation

pcm32
Copy link
Contributor

@pcm32 pcm32 commented Feb 19, 2018

This PR changes Grafana queries for pods and nodes to address problem describe in #1893.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. The email used to register you as an authorized contributor must be the email used for the Git commit.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 19, 2018
@k8s-reviewable
Copy link

This change is Reviewable

@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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 19, 2018
@pcm32 pcm32 changed the title Uses max instead of sum to avoid plotting variations at different timescale Uses max instead of sum in Grafana queries to avoid plotting variations at different timescale Feb 19, 2018
@DirectXMan12
Copy link
Contributor

cc @acobaugh

@acobaugh
Copy link
Contributor

I'm not sure I agree with the usage of max() instead of mean() here. If I want higher resolution, then I'm going to reduce my $interval. Using max() would actually give a skewed view of reality. Most of the grafana dashboards I see in the wild use mean(). I could maybe see an argument for separate set of metrics showing max over the selected interval, but that would make the graphs pretty crowded.

@jdumars
Copy link

jdumars commented Mar 2, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 2, 2018
@pcm32
Copy link
Contributor Author

pcm32 commented Mar 5, 2018

well I started it initially because it was set to sum, which had horrible results. I first used mean, but then it made more sense for most of my observations to use max. But, as long as it moves away from sum to either mean or max, I think is fine. I do think though that mean has an undesired softening effect, but I agree that max has also a bias.

@pcm32
Copy link
Contributor Author

pcm32 commented Mar 5, 2018

Maybe if some admin or project owner can let me know what they decide, I'm happy to change the branch to reflect it.

@acobaugh
Copy link
Contributor

acobaugh commented Mar 7, 2018

My personal vote is for mean(), but I'm still new to the project. Pretty much every single dashboard I've seen over the last few years of using grafana has used mean().

@pcm32
Copy link
Contributor Author

pcm32 commented Apr 14, 2018

I have switched all max to mean in the dashboards. Hopefully this can now be merged.

@pcm32 pcm32 changed the title Uses max instead of sum in Grafana queries to avoid plotting variations at different timescale Uses mean instead of sum in Grafana queries to avoid plotting variations at different timescale Apr 14, 2018
@acobaugh
Copy link
Contributor

/approve

@pcm32
Copy link
Contributor Author

pcm32 commented May 14, 2018

/assign @andyxning

@abelsromero
Copy link

Sorry to ask, but what's the reason not to merge this fix? The issue got closed by rotten but this is still pending

@pcm32
Copy link
Contributor Author

pcm32 commented Jul 5, 2018

I don't know.... I guess we need to ask another owner to approve it...

@pcm32
Copy link
Contributor Author

pcm32 commented Jul 5, 2018

Maybe @loburm could help us being an owner? he approved a recent PR. Sorry for the ping @loburm, I hope this is fine.

@DirectXMan12
Copy link
Contributor

Sorry about that. Don't know why @acobaugh was insufficient here.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2018
@pcm32
Copy link
Contributor Author

pcm32 commented Jul 17, 2018

Thanks @DirectXMan12! The tide robot apparently requires an lgtm label from what I read below, are you able to set that up?

@andyxning
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acobaugh, andyxning, DirectXMan12, pcm32

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 merged commit 78f6958 into kubernetes-retired:master Jul 17, 2018
@abelsromero
Copy link

Thanks a lot !!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants