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

Wen calculating 95th percentile, floor() maybe used instead of ceil() incorrectly #4963

Closed
htyypt opened this issue Oct 19, 2022 · 14 comments
Closed
Labels
bug Undesired behaviour confirmed Bug is confirm by dev team resolved A fixed issue
Milestone

Comments

@htyypt
Copy link

htyypt commented Oct 19, 2022

I have three sets of CACTI:

  1. 0.8.8a+php5.3.3+rrdtool1.4.9
  2. 0.8.8h+php5.6.40+rrdtool1.4.8
  3. 1.2.20+php7.2+rrdtool1.7.2

For example, the 5-minute data in September should be 12 * 24 * 30=8640 pieces of data, and 8640*0.05 is 432, which should be calculated from the 433rd piece of data.

But they are handled separately as follows:

  1. Some are 432, some are 433, I can't find their running rules
  2. Always 433 (correct)
  3. It is always 432 (the number is limited and the statistics are not accurate)

I looked at the graph_variables.php code for 0.8.8a and 0.8.8h, they are all the same, don't know why this is the result.

@htyypt htyypt added bug Undesired behaviour unverified Some days we don't have a clue labels Oct 19, 2022
@htyypt
Copy link
Author

htyypt commented Oct 19, 2022

Also, there is an error in the data time exported by 1.2.20:

Also, there is an error in the data time exported by 1.2.20:
2022-09 month data, according to the old version is 2022-09-01 0:05 - 2022-10-01 0:00, 1.2.20 is 2022-09-01 0:10 - 2022-10-01 0:05

image

@TheWitness
Copy link
Member

There is a rounding function on the element number, you are suggesting we use a ceil() instead of a floor() function then? One sample off?

@TheWitness
Copy link
Member

This is the bit of code from lib/graph_variables.php line # 300 (approximately). Do me a favor and change from floor() to ceil() and see if that makes the difference up. Let us know.

image

@htyypt
Copy link
Author

htyypt commented Oct 20, 2022

ceil():
image

floor():
image

export:
image

Looks like ceil() works;

@TheWitness
Copy link
Member

That's awesome, make my life much easier. Do you know how to create a pull request? If so, create one against the 1.2.x changing only this one file. If you need help, just chime in.

@htyypt
Copy link
Author

htyypt commented Oct 20, 2022

TheWitness, there is one more question for you.

1.2.20(rrdtool 1.7.2) There is an error in the export time:
The 2022-09 month data shows 2022-09-01 0:05 - 2022-10-01 0:00, but it is actually 2022-09-01 0:10 - 2022-10-01 0:05.

But when I changed rrdtool to version 1.48, the problem disappeared, how can I go about it.

image
image

@TheWitness
Copy link
Member

Is your browsers time zone different than the servers? This is a regression when we introduced that feature. @thurban is working on a way to disable that until we get the feature more unified across not only Core Cacti, but the plugin architecture. This should be resolved before too long. Issue is here:

#4926

@htyypt
Copy link
Author

htyypt commented Oct 20, 2022

My browser is in the same timezone as the server and it doesn't seem to be the same problem.

@TheWitness
Copy link
Member

Okay, but one issue per ticket. Can you do the pull request?

@htyypt
Copy link
Author

htyypt commented Oct 20, 2022

Regarding the first question, I have already submitted. I don't know if it is correct, please take a look.

@TheWitness
Copy link
Member

So, you made a few mistakes. Easy to fix though. What you did was try to merge develop in to 1.2.x, which is the wrong way. What you need to do is switch to the 1.2.x branch before making your changes. If using the command line, you would do this:

git checkout 1.2.x
vim lib/graph_variables.php

If using the Desktop Client, then there is a dropdown as shown in the image below. Simply change to 1.2.x and make your changes. I find the Desktop Client pretty convenient.

image

@htyypt
Copy link
Author

htyypt commented Oct 21, 2022

Sorry, please help to change it this time, I will learn.

@TheWitness
Copy link
Member

No problem, it's queued up, but I'm heading out for vacation for next week or so. Get it done when I get back. In the mean time maybe @xmacan can pull it together. I'll be monitoring from my phone.

TheWitness added a commit that referenced this issue Nov 5, 2022
Will the calculation rules of 95nth_percentile involve php and rrdtool versions
@TheWitness TheWitness added confirmed Bug is confirm by dev team and removed unverified Some days we don't have a clue labels Nov 5, 2022
@TheWitness TheWitness added this to the v1.2.23 milestone Nov 5, 2022
@TheWitness TheWitness added the resolved A fixed issue label Nov 5, 2022
@TheWitness
Copy link
Member

Resolved.

TheWitness added a commit that referenced this issue Dec 3, 2022
Make Tree the unlock process not have to rebuild the page
@netniV netniV changed the title Will the calculation rules of 95nth_percentile involve php and rrdtool versions? Wen calculating 95th percentile, floor() maybe used instead of ceil() incorrectly Dec 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Undesired behaviour confirmed Bug is confirm by dev team resolved A fixed issue
Projects
None yet
Development

No branches or pull requests

2 participants