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

[DRAFT] luci-app-statistics: DSL graphs update #6798

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Weissnix4711
Copy link

@Weissnix4711 Weissnix4711 commented Dec 30, 2023

  • Split up SNR and attenuation into two graphs
  • Changes type of attenuation instances to gauge. Not sure if this is the correct type for attenuation data, but snr didn't quite fit either.
  • Reorder things to be in the order: down/near first and positive y, up/far second and flipped to negative y
  • Sensible units, formatting and totals
  • Add more RETX errors
  • CRC errors
  • Errored seconds

I'm still on 22.03, due to the issues with the HH5a, hence I cannot properly test this. My current test setup uses a collectd-exec and a shell script I hastily threw together which calls ubus to gather the DSL metrics. Crucially, not the same as the actual lua script now in the collectd package, so there is plenty of room for things to go wonky.

Not sure if the graph titles work correctly. %pi is meant to be plugin instance. For what I threw together using collectd-exec ages ago, this is dsl0. I'm not honestly sure how the lua script does this.

I think I have added all the necessary changes to the lua script for gathering the extra data, but if someone could test that would be greatly appreciated. openwrt/packages#23011

Paging @bluebrother because I know I'm rewriting much of their code.

Screenshots:
1
2
3
8
esn
crc
retx

Not that there's much to show for the errors lol. Nor flags, my shell script didn't really deal with bools well

@systemcrash
Copy link
Contributor

So does attenuation ever become negative (i.e. amplification)?
Uptime probably should not be a monotonically increasing graph, but I can't think how else it would be better, if collectd is on the system. If the system is down, it cannot record up or down time.

Does the relevant graph depend on its position in an array?

Hmm. This is from a hh5a? Not to derail this discussion, but how was it to install that device? Did you require serial console as per the wiki?

@Weissnix4711
Copy link
Author

So does attenuation ever become negative (i.e. amplification)?

No, to my knowledge it could never be negative. This is why I'd suggest it's good to plot both up and down on the same graph, with up simply being flipped to the negative side.

Uptime probably should not be a monotonically increasing graph, but I can't think how else it would be better

Neither could I. I've basically just left uptime as it was before, with addition of some small formatting tweaks.

Does the relevant graph depend on its position in an array?

How you mean, sorry?

Did you require serial console as per the wiki?

Yes, precisely. I've reflashed multiple HH5a's (or rebranded clones) by soldering directly to the test points on the board.

Copy link
Contributor

@bluebrother bluebrother left a comment

Choose a reason for hiding this comment

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

Can you explain the reason for these changes? To me these are just style changes, not fixes, and other files in the same folder use the formatting this file originally had: use " for strings, not have a whitespace betwen function and (, not use a trailing comma on the last item in a list.

Did you editor auto-format and introduce those changes?

@Weissnix4711
Copy link
Author

Admittedly, yes this PR was thrown together rather hastily. There were a lot of stylistic changes in there which were enforced by my IDE's auto formatting. I have just reverted the single quotes and random whitespace. Hopefully that's a bit better and makes the diff more readable.

@systemcrash
Copy link
Contributor

Hmm, after a bit more thought, perhaps uptime can simply be displayed as a clock, years, months, days, (weeks?), hours, minutes, seconds, rather than a graph whose limit is axis height will effectively never change shape, only the axis units crammed in will get further between. I don't think rrdtool will do this, but just a large rendered string at top or bottom would be fine.

@Weissnix4711
Copy link
Author

Not sure I agree to be honest. It can be useful to see the history of when the line previously went down, which a simple counter wouldn't show.

However, I have given it a thought and yes you're right, we need something better than graphing uptime in seconds. Newer versions of rrdtool have the ability to format a time duration into a number of days, hours, minutes, etc. See the docs. This would be great, but we're still stuck on v1.0.x (newest is v1.8.0, not sure when the time/duration formatting was introduced). I have opened an issue in the packages repo: openwrt/packages#23087.

@systemcrash
Copy link
Contributor

I think things like this were noted earlier issue #6139 - some of those are not simple to fix.

which a simple counter wouldn't show.

A counter would reset to zero (including everything else on platforms that don't save the graph data to storage), but it's not obvious without seeing the line start from zero again.

@Weissnix4711 Weissnix4711 marked this pull request as ready for review February 16, 2024 00:25
@systemcrash
Copy link
Contributor

@Weissnix4711

However, I have given it a thought and yes you're right, we need something better than graphing uptime in seconds

How about instead of uptime, just graph line up status, which is basically boolean. That clearly shows when the line is not up.

@systemcrash
Copy link
Contributor

ping

@Neustradamus
Copy link

@Weissnix4711: Have you progressed on this PR?

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.

4 participants