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

Clients over time popover gets cut off #603

Closed
3 tasks done
WaLLy3K opened this issue Oct 14, 2017 · 49 comments
Closed
3 tasks done

Clients over time popover gets cut off #603

WaLLy3K opened this issue Oct 14, 2017 · 49 comments

Comments

@WaLLy3K
Copy link
Contributor

WaLLy3K commented Oct 14, 2017

In raising this issue, I confirm the following:

How familiar are you with the codebase?:

3


untitled-2

untitled-3

The new Clients over Time popover gets cut off, rendering it basically unusable for determining which client made a huge spike in queries.

@andreskrey
Copy link
Contributor

Same thing happens with the "Forward Destinations over Time" when you have multiple DNS servers.

@PromoFaux
Copy link
Member

In raising this issue, I confirm the following: {please fill the checkboxes, e.g: [X]}

How familiar are you with the the source code relevant to this issue?:

10


Expected behaviour:

Tooltips should render outside of the canvas

Actual behaviour:

Tooltips only render within the canvas.

Steps to reproduce:

Mouseover a chart, and look at it.

Troubleshooting undertaken, and/or other relevant information:

A possible fix is to use External (Custom) Tooltip

Some discussion on it here : chartjs/Chart.js#622

If I can get some time, I'll take a look into it.

@DL6ER
Copy link
Member

DL6ER commented Jan 5, 2018

This article on Discourse is suggesting to increase the height of the graphs notable to mitigate this issue, what do you think @pi-hole/admin ?

@PromoFaux
Copy link
Member

I mean, yeah, that's one way of doing it, but I think it would be nicer to go down the custom tooltip path. That said, I've had no time to work on that myself lately!

@AzureMarker
Copy link
Contributor

With enough clients, it will still be an issue with that workaround.

@excyruss
Copy link

excyruss commented Jan 9, 2018

It helps, but you get repeating similar colours too (which is probably another issue, sorry).

pihole

We need fewer devices in the home but I doubt that's the trend :)
It may be a good idea to order the colours in the tool tip in the same order as the graph

@borpin
Copy link

borpin commented Feb 7, 2018

Could the position of the legend be made static on the RHS of the chart, but the data fed from where the mouse is in terms of updating the legend?

@AzureMarker
Copy link
Contributor

I haven't check that case, but it would depend on if the library supports it. I would guess not because it renders everything in a canvas

@borpin
Copy link

borpin commented Feb 9, 2018

Couple of thoughts:

  • The 'Queries over last 24 hours' chart tooltip appears on top or to left of mouse location which is better.
  • The 'Clients (over time)' chart it always is displayed below the mouse location. If it too was to the left or above, that would help.

On the 'Clients (over time)' chart, removing those clients with no data would help as well.

An alternative approach could be to make the title of the chart clickable, taking the user to the chart on a single page, the chart could be much bigger (with more canvas space below the axis which might help.

Could something like this be done? http://hoboman313.github.io/chartsjs-demo/

This http://www.flotcharts.org/flot/examples/tracking/index.html is quite a neat solution with a fixed legend though it uses flot and I have not been able to find a chartjs solution to match it.

@maciboy
Copy link

maciboy commented Mar 9, 2018

@excyruss
The issue with too many clients can be diminished by filtering all clients with currently 0 requests.
In your case only 6 clients would be shown then…
Of course this is not a full solution, but with the assumption that not all clients are active in every timespan, it would at least improve the situation 😄

Without having too much knowledge of the code, I’d assume it’s that code responsible:
index.js: http://pi.hole/admin/scripts/pi-hole/js/index.js:807 ff.:
bildschirmfoto 2018-03-09 um 19 48 40

Filtering „if (tooltipItems.yLabel != 0)“ would do the job, or am I wrong?

@kh0505
Copy link

kh0505 commented Mar 12, 2018

@maciboy
Thanks. Updating that section of code strips out the 0 count clients which I don't care about anyway. It's not a perfect solution but makes most areas on my graph not cut off the tooltip as I have around 18 clients. This has been bugging me for quite a while. I have researched this and other applications using Chart.js have the same issue. As PromoFaux suggested above it appears that a Custom Tooltip is the only real solution. My coding skills are a little to rusty for that these days but for now this solution will suffice.

Easy enough for anyone to update on their own.

                    label: function(tooltipItems, data) {
                            if(tooltipItems.yLabel != 0) {
                                return data.datasets[tooltipItems.datasetIndex].label + ": " + tooltipItems.yLabel;
                            }
                    }

@kh0505
Copy link

kh0505 commented Mar 17, 2018

This is how to get a customtooltip that renders outside of the clients over time graph. I didn't apply it to the queries over time graph. Also, if you implemented the stripping out of 0 count clients above I would suggest undoing that as you will end up with blank lines with this new code. I am going to enjoy the rest of my Saturday rather than fiddling with that right now... The below is pretty much a hack of me googling and researching but it works. Not as pretty as it doesn't have the triangle fly away on the tooltip but it works. I am sure there is some process to officially submit it on github but I don't know if I am going to work on this anymore since I got it to do what I wanted. So do what you will with it.

piholetooltipfix.txt

@maciboy
Copy link

maciboy commented Mar 17, 2018

Would love to see some kind of adoption of @kh0505 modification!

Maybe you can share a screenshot, of how your custom tooltip looks in usage?

@kh0505
Copy link

kh0505 commented Mar 17, 2018

image

@PromoFaux
Copy link
Member

Good stuff @kh0505! I think this is one of those things we've just been putting off and off! You're free to submit a pull request against the repository (devel branch, please) if you'd like to.

@maciboy
Copy link

maciboy commented Mar 18, 2018

I managed to edit both files the way @kh0505 explained, for those of you wanting to test it:
Tooltip.zip

@91ajames
Copy link

91ajames commented Mar 20, 2018

Thank you so much @kh0505 , hope this gets added in the next update. honestly, they should put this in now, right away. but of course some possible breaks may endure and if any security risk but I doubt it.

@PromoFaux
Copy link
Member

We'll add it in, just giving @kh0505 a grace period should he wish to submit it himself. However, we will now add it in and credit him in the commit message

DL6ER added a commit that referenced this issue Mar 20, 2018
@thewraith2008
Copy link

The issue can be mitigated by applying by hand the changes to the 2 files (plain text) indicated in the above posts. This is a low priority item that the DEV team will get to one day, for now doing it for yourself will be the only option if you really need something. Also if you make the changes, you can give feedback to assist with its improvement.

@PromoFaux
Copy link
Member

It's in progress:
#700

@91ajames
Copy link

91ajames commented Apr 13, 2018

was working for a while until last week or so, it reverted back to half screen unseeable address's.

@thewraith2008
Copy link

@91ajames if you have done an Update (pihole -up) or Reconfigure/Repair (pihole -r) of Pihole in the last week or so, this will overwrite (restore to release default) any manual changes that where made by hand.

@AzureMarker
Copy link
Contributor

The PR has been merged into development

@bryceacc
Copy link

Sorry for the stupid question but how do I modify these files to implement this fix myself? I stopped lighttpd but the files are still not writable and even if I force a write and then restart lighttpd or the entire pi, the changes don't actually get reflected in the admin page once it starts up again. How do I modify the files in /var/www/html/admin/ ?

@AzureMarker
Copy link
Contributor

I would not suggest modifying the files yourself. They are writable if you use sudo.

@bryceacc
Copy link

Well that's the thing, I transferred the modified files and sudo cp'ed them into the right places. cat on the index.js and .php file both show the correct modifications but after a restart, the admin console is not fixed. Chrome tools show the source code and they are unmodified. Is there a different directory that the web server is running from?

I modified these two files to kh0505's instruction:

/var/www/html/admin/scripts/pi-hole/js/index.js
/var/www/html/admin/index.php

@dschaper
Copy link
Member

Sounds like a bad SDCard. Try touching a file in the home directory, rebooting and see if that file still exists after the reboot.

@bryceacc
Copy link

touch test is fine, I also cat checked the modifications before and after reboot so I know they are written. Unless sudo reboot isn't performing a complete restart?

Everything else about the pi and pihole has been amazing, I just turned it into the DHCP server though and that grew my client list to the correct amount of clients I have around which is way too many for the small graph. Even just putting in the 0 requests check helps a lot, and I can modify that from chrome tools but it would be nice to be a persistent change

@dschaper
Copy link
Member

So cat /var/www/html/admin/index.php shows your changes but viewing the page in a browser and F12 view source does not match the cat version?

@dschaper
Copy link
Member

dschaper commented May 31, 2018

How about sudo systemctl --no-pager --full status lighttpd.service and cat /etc/lighttpd/lighttpd.conf?

@bryceacc
Copy link

Correct, cat on the two index files shows the changes at all times after multiple reboots, I look at the source displayed in my browser and it's the original unmodified code that I wanted changed.

pi@raspberrypi:~ $ sudo systemctl --no-pager --full lighttpd.service
Unknown operation lighttpd.service.

The other command looks like a good ol config file but messes up markdown so I won't insert that. The parts that specify directories and files are:

server.document-root = "/var/www/html"
server.error-handler-404 = "pihole/index.php"
server.upload-dirs = ( "/var/cache/lighttpd/uploads" )
server.errorlog = "/var/log/lighttpd/error.log"
server.pid-file = "/var/run/lighttpd.pid"
server.username = "www-data"
server.groupname = "www-data"
server.port = 80
accesslog.filename = "/var/log/lighttpd/access.log"
accesslog.format = "%{%s}t|%V|%r|%s|%b"
index-file.names = ( "index.php", "index.html", "index.lighttpd.html" )

@dschaper
Copy link
Member

It's sudo systemctl --no-pager --full status lighttpd.service I dropped a word. And you can paste in the full config and wrap the text with ``` backticks to preserve the original formatting.

@dschaper
Copy link
Member

Also, may not make a difference but have you CTRL-F5 in the browser to hard clear any cache?

@bryceacc
Copy link

oh no... 🤦 hard refresh works... thanks for your help!

@Melantrix
Copy link

So this should have been in the next release, according to the label, but i have just updated to v4.0 but the graph error is still there :( Is it correct that the fix has not been released, or am i missing something?

@DL6ER
Copy link
Member

DL6ER commented Aug 6, 2018

@Melantrix Please try a full refresh (Ctrl + F5 or Shift + Ctrl + R). Your browser might still be caching the old version of the scripts.

@Melantrix
Copy link

Melantrix commented Aug 6, 2018

I've done that but after manually deleting chrome and reinstalling it. (NB: I am on mobile; Chrome on Android)

@DL6ER
Copy link
Member

DL6ER commented Aug 6, 2018

@Melantrix Do you mean that it still doesn't work?

@Melantrix
Copy link

Oh I'm sorry, that message was absolutely unclear xD it works now, but only after reinstalling the app..

@AzureMarker
Copy link
Contributor

By uninstalling and reinstalling the app, it cleared its cached data.

This issue is fixed in v4.0.

@pralor-bot
Copy link

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

https://discourse.pi-hole.net/t/pi-hole-v4-0-released-with-ftldns-improved-blocking-modes-regex-docker-and-more/11462/1

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

No branches or pull requests