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

Replace temperature display by query frequency #3139

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Sep 7, 2024

What does this implement/fix?

See title.

image

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

@DL6ER DL6ER requested a review from a team September 7, 2024 20:30
@yubiuser
Copy link
Member

yubiuser commented Sep 7, 2024

Personally, I don't like 'Queries per minute'. I think it is confusion when the 'scale' changes depending on the number of events. Additionally, fractions of q/s doesn't make sense to me. I suggest to just round it to integer and show '<1 q/s' for really low values.

@rdwebdesign
Copy link
Member

rdwebdesign commented Sep 7, 2024

I don't like 'Queries per minute'.

Actually, initially I though this was a good idea, specially in systems with a very low query frequency, but changing q/s to q/m is too subtle (only one letter is changed) and users will not notice the unit change.

If we decide to keep 'Queries per minute' for cases with small frequencies, I still think we should use q/min.

Another suggestion:

I think it is confusion when the 'scale' changes depending on the number of events.

We can keep the change between 'Queries per second' and 'Queries per minute', but we could also change the color (to blue or something else) when the unit changes to "per minute" to make the change more visible.

@DL6ER
Copy link
Member Author

DL6ER commented Sep 7, 2024

With your two suggestions

  • always show q/s (no conversion to per-minute), and
  • show < 1 q/s for smaller frequencies
    we can go ahead and simply hard-code the < 1 q/s string and don't bother with actually computing the real number on most systems.

You will have to see more than 24*60*60 = 86,400 per 24 hours to get the value 1 q/s and already need double that number (172,800 queries in 24 hrs) to get a 2 q/s.


So should it always be per-min ?

@rdwebdesign
Copy link
Member

So should it always be per-min ?

I think this is a good idea.

Maybe we could reverse the strategy and use a different unit only in extreme cases, if there are too many queries per minute (like this), to avoid a text "overflow", but this is very uncommon.

@yubiuser
Copy link
Member

yubiuser commented Sep 8, 2024

You will have to see more than 246060 = 86,400 per 24 hours to get the value 1 q/s and already need double that number (172,800 queries in 24 hrs) to get a 2 q/s.

I'm not sure if I can follow your logic here. Sure the calculation is correct, but the info shown is not intended to show the 24 hour average, but the 'current' query stream.
But I agree, maybe it's better to show queries/min by default and only above a threshold for high load switch to queries/sec

@DL6ER
Copy link
Member Author

DL6ER commented Sep 9, 2024

I'm not sure if I can follow your logic here. Sure the calculation is correct, but the info shown is not intended to show the 24 hour average, but the 'current' query stream.

However, I have seen in many screenshots that the query "load" is rather even, e.g., on my own Pi-hole, I see
image

so I'll barely see 1 q/s while for most it way always be < 1 q/s - okay, maybe +/- for a few seconds at peak times every day

@yubiuser
Copy link
Member

yubiuser commented Sep 9, 2024

Your distribution is quite distinct from mine...

2024-09-09_21-01

__

So where are we going from here?
Default to queries/min and switch to queries/sec above e.g. 100 q/sec (for really large installations or loop detection)?

@PromoFaux
Copy link
Member

wildcard idea - make it configurable... either /s or /min dependent on user preference.

…han 100 queries are received per minute (~ 1.6q/s)

Signed-off-by: DL6ER <[email protected]>
@DL6ER
Copy link
Member Author

DL6ER commented Sep 9, 2024

More configurable means more likeliness to break and a lot more maintenance. I adopted the idea suggested above.

image

// Determine number of fraction digits based on the frequency
// - 0 fraction digits for frequencies > 10
// - 1 fraction digit for frequencies between 1 and 10
// - 2 fraction digits for frequencies < 1
Copy link
Member

Choose a reason for hiding this comment

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

I still think we should only go with full integer values and "<1" only.

Copy link
Member

Choose a reason for hiding this comment

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

I think showing decimal places only for small numbers (< 10) is fine.
We already show decimal places for the CPU and MEM.

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

Successfully merging this pull request may close these issues.

4 participants