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] collectd: minor dsl lua script changes #23011

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

Conversation

Weissnix4711
Copy link

@Weissnix4711 Weissnix4711 commented Dec 30, 2023

Maintainer: me / @<github-user> (find it by checking history of the package Makefile)
Compile tested: (put here arch, model, OpenWrt version)
Run tested: (put here arch, model, OpenWrt version, tests done)

Description:

  • Change attenuation types from snr to gauge. Not sure if this is really the best option, just wanted to differentiate it from the already existing snr margin, because it really isnt to do with snr
  • Add rx_corrected, rx_uncorrected_protected, crc_p and crcp_p errors
  • If name and type are identical (currently only relevant for snr), the name will be ignored. Ie. snr-snr_down becomes snr-down. I think this looks neater, but maybe that's just me.

Open to criticism.

Primarily for openwrt/luci#6798

Signed-off-by: Thomas Aldrian <aldrian.thom@proton.me>
Signed-off-by: Thomas Aldrian <aldrian.thom@proton.me>
Signed-off-by: Thomas Aldrian <aldrian.thom@proton.me>
local metric = build_metric(name, direction)
if information["type"] == "bool" then

if name = type then
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intended to be an assignment? Looks like it should be == to me.

Also, I'm not a fan of this special case. I don't see much benefit but unnecessary extra complexity, but that might be just me :)

Copy link
Author

Choose a reason for hiding this comment

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

Yup, you're right. That's just a typo. As I said, I haven't yet tested these changes lol.

You're probably right. I mean, it doesn't really make any difference so it could also just be a waste of time to put any more thought into it. Ultimately, it ain't my call. If you want this change removed, I'll happily do so.

Signed-off-by: Thomas Aldrian <aldrian.thom@proton.me>
@systemcrash
Copy link
Contributor

If you think these look good - mark this as ready

@Weissnix4711 Weissnix4711 marked this pull request as ready for review February 16, 2024 00:06
@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