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

Better interval computation for Long-term data -> Graphics page #2132

Merged
merged 1 commit into from
Mar 12, 2022

Conversation

rubpa
Copy link
Contributor

@rubpa rubpa commented Feb 20, 2022

Signed-off-by: rubpa rubpa@users.noreply.github.com

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

The Long-term data -> Graphics page showed graphs with very "unrounded" intervals as in the screenshots below.

image
API call for above was: admin/api_db.php?getGraphData&from=1645209000&until=1645276215.962&interval=336.07980999946597

image
API call for above was: admin/api_db.php?getGraphData&from=1644671415.963&until=1645276215.963&interval=3024

How does this PR accomplish the above?:

This PR tries to still keep approx 200 intervals but by choosing more "rounded" values like 10 mins, 20 mins, 30 mins, 1 hr, etc. based on the selected duration.

What documentation changes (if any) are needed to support this PR?:

Documentation changes are not required.

@rdwebdesign
Copy link
Member

rdwebdesign commented Feb 25, 2022

I understand what you're trying to achieve (human readable intervals - integer intervals), but your current code might cause problems in certain situations.

Example:
Selecting "Custom Range" with a narrow range (eg: 2 hours), "600 seconds" is too big and you get few values.
PR2132

My suggestion is to allow intervals smaller than 600 seconds (since you want around 200 values):

  const intervals = [10, 20, 30, 60, 120, 180, 300, 600, 900, 1200, 1800,
    3600, 3600 * 2, 3600 * 3, 3600 * 4, 3600 * 5, 3600 * 6,
    3600 * 8, 3600 * 12, 3600 * 24, 3600 * 24 * 7, 3600 * 24 * 30
  ];

@rubpa
Copy link
Contributor Author

rubpa commented Mar 2, 2022

Thanks @rdwebdesign! I've updated as per comments. Did not take 5hrs since it does not "slot" well into 24hrs. I've also rebased to latest devel.

@rdwebdesign
Copy link
Member

pi-hole/AdminLTE repo uses Prettier to verify code format.
Prettier encountered some code formatted differently than expected. Could you please resolve these issues?

Break this long array declaration:

  const intervals = [
    10,
    20,
    30,
    60,
    120,
    180,
    300,
    600,
    900,
    1200,
    1800,
    3600,
    3600 * 2,
    3600 * 3,
    3600 * 4,
    3600 * 6,
    3600 * 8,
    3600 * 12,
    3600 * 24,
    3600 * 24 * 7,
    3600 * 24 * 30,
  ];

And remove unnecessary parentheses:

    var err = Math.abs(1 - duration / (num * intervals[i]));
    // pick the interval with least deviation

@rubpa
Copy link
Contributor Author

rubpa commented Mar 5, 2022

pi-hole/AdminLTE repo uses Prettier to verify code format. Prettier encountered some code formatted differently than expected. Could you please resolve these issues?

Done! Did not rebase this time. Hope that's ok.

@rdwebdesign
Copy link
Member

Prettier is still complaining - Tests / Node (pull_request). Please, click on "Details" link to identify the errors.
image

I think your comment has trailing spaces or tabs (sometimes prettier is annoying).

@rubpa
Copy link
Contributor Author

rubpa commented Mar 6, 2022

Hopefully it's fine now! :)

@rdwebdesign
Copy link
Member

xo is complaining (xo runs "eslint").

The test returned "Error - Parsing error: Unexpected token i".

@rdwebdesign
Copy link
Member

I think eslint configuration is expecting a different standard.

For now, try remove let to see if passes the test.

@rdwebdesign
Copy link
Member

I did a few tests.

I replaced let and the previous error is gone, but there are new errors:

Error: /scripts/pi-hole/js/db_graph.js: line 99, col 17, Error - Number.MAX_SAFE_INTEGER() is not supported in IE 11 (compat/compat)
Error: /scripts/pi-hole/js/db_graph.js: line 100, col 3, Error - The body of a for-in should be wrapped in an if statement to filter unwanted properties from the prototype. (guard-for-in)
Error: /scripts/pi-hole/js/db_graph.js: line 108, col 5, Error - Expected blank line before this statement. (padding-line-between-statements)
Error: /scripts/pi-hole/js/db_graph.js: line 110, col 3, Error - Expected blank line before this statement. (padding-line-between-statements)

@rubpa
Copy link
Contributor Author

rubpa commented Mar 6, 2022

Where can I find the eslint configuration so that I can try locally?

@rdwebdesign
Copy link
Member

The tests are configured in https://github.com/pi-hole/AdminLTE/blob/master/package.json

You just need to run

npm run prettier:check && npm run xo

rdwebdesign
rdwebdesign previously approved these changes Mar 6, 2022
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Please do not use for...in with array iteration.
https://stackoverflow.com/questions/500504/why-is-using-for-in-for-array-iteration-a-bad-idea

Use something simple like for (i=0; i<intervals.length; i++) {

Signed-off-by: rubpa <rubpa@users.noreply.github.com>
Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

LGTM

@yubiuser yubiuser requested a review from rdwebdesign March 12, 2022 05:59
@yubiuser yubiuser merged commit 32aa7ab into pi-hole:devel Mar 12, 2022
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-15-web-v5-12-and-core-v5-10-released/54987/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants