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

Upstream donut: Low values strike yet again #2500

Closed
saint-lascivious opened this issue Jan 18, 2023 · 7 comments · Fixed by #2502
Closed

Upstream donut: Low values strike yet again #2500

saint-lascivious opened this issue Jan 18, 2023 · 7 comments · Fixed by #2502
Labels

Comments

@saint-lascivious
Copy link

saint-lascivious commented Jan 18, 2023

Versions

  • Pi-hole: 5.15
  • AdminLTE: 5.18.1
  • FTL: 5.20.1

Platform

  • OS and version: Ubuntu 22.10
  • Platform: RPi 3

Expected behavior

Accurate upstream donut slices.

Actual behavior / bug

Inaccurate upstream donut slices.

Steps to reproduce

Steps to reproduce the behavior:

  1. Have at least two upstreams with very low values, less than 1, but equal to or greater than 0.1 (other variations of this issue affected values less than 0.1, the fix for that appeared to be removing the tooltip in that case).

Screenshots

Screenshot_2023-01-18-17-44-13-67.jpg
Screenshot_2023-01-18-17-44-52-70.jpg
Screenshot_2023-01-18-17-45-29-25.jpg

Additional context

In the provided screenshots you can see two values of 0.1 being represented as 40% and 60%, instead of 50% each (0.1 and 0.1 are equal values).

@saint-lascivious
Copy link
Author

saint-lascivious commented Jan 18, 2023

Supplementary: Trying to understand the very low apparent cache usage, are responses returned from stale cache not counted as cache hits?

Stale cache hits appear to have a unique identifier in the database but I'm not sure how this is represented in the APIs/stats.

Edit: Additional minor quibble, should it not be cache as opposed to cached?

The upstream is cache, cached is what had to happen before it could be an upstream.

This possibly begs a deeper question as to whether cache or block responses even belong in an upstream graph.

@yubiuser
Copy link
Member

Thanks for your report. I see where your confusion comes from, however, I think this is not a "bug" itself but an issue with rounding really small values. We use this code to display/calculate the tooltip:
https://github.com/pi-hole/AdminLTE/blob/30668d4d6742136e2c0ae7641207f68ff69c99d7/scripts/pi-hole/js/index.js#L726-L755

All values are rounded to 0.1 precision. One of your upstrams probably has 0.12 and the other one 0.08 % of all queries. This is rounded in both cases to 0.1. But regarding the share of the "shown %", it's calculated with the non-rounded values: 0.12/(0.12+0.08)x100=60%

@saint-lascivious
Copy link
Author

saint-lascivious commented Jan 18, 2023

One of your upstrams probably has 0.12 and the other one 0.08 % of all queries. This is rounded in both cases to 0.1. But regarding the share of the "shown %", it's calculated with the non-rounded values: 0.12/(0.12+0.08)x100=60%

Yes, that appears to be what's happening here. It occurred to me I could just look at my Munin plugin to confirm.

So, what then if anything, is the solution to this? I understand it, but it really doesn't feel "correct". Though, neither do any workarounds seem any more correct. An extra degree of precision could maybe make it obvious what's happening - but it wouldn't be needed for all values and it seems difficult or impossible to predict where these weird edge cases pop up.

Any input on the supplementary question would be appreciated also.

@yubiuser
Copy link
Member

An extra degree of precision could maybe make it obvious what's happening - but it wouldn't be needed for all values and it seems difficult or impossible to predict where these weird edge cases pop up.

This is the way to go I think. I'll implement a version that will increase precision of the labels to 0.01 in case the sum of all shown items is less then 1%.

@yubiuser
Copy link
Member

Please try pihole checkout web double_precision

@saint-lascivious
Copy link
Author

Looks good on this end.

@yubiuser
Copy link
Member

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

Successfully merging a pull request may close this issue.

2 participants