Skip to content

Sensors - Allow changing RRD type in YAML and PHP sensor discovery#14208

Merged
murrant merged 99 commits intolibrenms:masterfrom
PipoCanaja:counter_type
Oct 25, 2022
Merged

Sensors - Allow changing RRD type in YAML and PHP sensor discovery#14208
murrant merged 99 commits intolibrenms:masterfrom
PipoCanaja:counter_type

Conversation

@PipoCanaja
Copy link
Copy Markdown
Contributor

@PipoCanaja PipoCanaja commented Aug 15, 2022

Replaces #13854

A new DB column in sensors stores the RRD type (which defaults to GAUGE as it is currently). This is a draft currently. Will update this desc as it reaches a "runnable" status.

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.
  • If my Pull Request makes discovery/polling/yaml changes, I have added/updated test data.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@PipoCanaja PipoCanaja self-assigned this Aug 15, 2022
@PipoCanaja PipoCanaja added Discovery Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ labels Aug 15, 2022
@github-actions
Copy link
Copy Markdown

Please add test data so we can ensure your change is not broken in the future.
Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@murrant
Copy link
Copy Markdown
Member

murrant commented Aug 16, 2022

Seems reasonable, do any changes need to be made to the graphs?

@PipoCanaja
Copy link
Copy Markdown
Contributor Author

PipoCanaja commented Aug 16, 2022

Seems reasonable, do any changes need to be made to the graphs?

Not necessarilly. May be some text/graphical changes to add "per second" to the unit, etc. Will see how it goes with tests.

@PipoCanaja

This comment was marked as outdated.

@PipoCanaja PipoCanaja force-pushed the counter_type branch 2 times, most recently from aa62dc7 to 96d27ac Compare September 7, 2022 08:58
@PipoCanaja
Copy link
Copy Markdown
Contributor Author

@murrant Whatever I do, save-test-data cannot provide me 100% of files that would pass correctly the tests. Even on my own server, with same DB and same settings, they do not succeed. Would you have any suggestions ?

@PipoCanaja PipoCanaja force-pushed the counter_type branch 3 times, most recently from b6c3031 to e9bfaa3 Compare September 8, 2022 16:24
@PipoCanaja
Copy link
Copy Markdown
Contributor Author

@librenms/reviewers : tried my best with tests but still fighting to get it right : save-test-data runs as good as it can, and provides a result that makes sense.
lnms dev:check -o arista-mos runs well on my VM.
But we have an error here in the PR tests:
https://github.com/librenms/librenms/runs/8259961537?check_suite_focus=true

@murrant
Copy link
Copy Markdown
Member

murrant commented Sep 9, 2022

Sorry this is a nightmare to update :/

@PipoCanaja
Copy link
Copy Markdown
Contributor Author

Hi @murrant @Jellyfrog
I am calling for help :)
On the precise test that is currently failing currently, I don't get the issue :
tests/data/arista-mos_metamux48-c-0-16.json

First, this :
image

And then this:
image

Do you have any tips ?

Thx

@Jellyfrog
Copy link
Copy Markdown
Member

It except sensors but doesn't find any for that device.

@PipoCanaja
Copy link
Copy Markdown
Contributor Author

It except sensors but doesn't find any for that device.

But looking at the capture, you see that the data is actually found :) That's my problem. It finds it but complains it doesn't ...

@PipoCanaja PipoCanaja changed the title WIP - Sensors - Allow changing RRD type in YAML and PHP sensor discovery Sensors - Allow changing RRD type in YAML and PHP sensor discovery Oct 9, 2022
@murrant murrant added this to the 22.11.0 milestone Oct 11, 2022
@murrant murrant removed the Needs Tests 🦄 https://docs.librenms.org/Developing/os/Test-Units/ label Oct 11, 2022
@PipoCanaja
Copy link
Copy Markdown
Contributor Author

Hi @librenms/reviewers
Looks like it is OK now. Only have to keep up with master each time a JSON file is added or updated.
Let me know your opinion on this.

@murrant murrant merged commit 8e3fe22 into librenms:master Oct 25, 2022
@Jellyfrog
Copy link
Copy Markdown
Member

Might miss something, but when running ./scripts/save-test-data.php -o apc -v sua750i, it breaks on:

Error polling sensors module for 127.1.6.1. LibreNMS\Exceptions\InvalidRrdTypeException: is not valid, must be: GAUGE | DERIVE | COUNTER | ABSOLUTE | DCOUNTER | DDERIVE in librenms/LibreNMS/RRD/RrdDefinition.php:122

because the $sensor doesnt have a rrd_type set:

array:25 [
  "sensor_id" => 107
  "sensor_deleted" => 0
  "sensor_class" => "state"
  "device_id" => 13
  "poller_type" => "snmp"
  "sensor_oid" => ".1.3.6.1.4.1.318.1.1.1.4.1.1.0"
  "sensor_index" => "0"
  "sensor_type" => "upsBasicOutputStatus"
  "sensor_descr" => "Output Status"
  "group" => null
  "sensor_divisor" => 1
  "sensor_multiplier" => 1
  "sensor_current" => 2.0
  "sensor_limit" => null
  "sensor_limit_warn" => null
  "sensor_limit_low" => null
  "sensor_limit_low_warn" => null
  "sensor_alert" => 1
  "sensor_custom" => "No"
  "entPhysicalIndex" => null
  "entPhysicalIndex_measured" => null
  "lastupdate" => "2022-10-31 18:00:39"
  "sensor_prev" => null
  "user_func" => null
  "new_value" => "2"
]

@PipoCanaja
Copy link
Copy Markdown
Contributor Author

Might miss something, but when running ./scripts/save-test-data.php -o apc -v sua750i, it breaks on:

did you run the migrations ? I would be the DB is not in the correct state.

@PipoCanaja PipoCanaja deleted the counter_type branch October 31, 2022 18:24
@Jellyfrog
Copy link
Copy Markdown
Member

Oh. No :) it's probably that then. I'll recheck tomorrow

@PipoCanaja
Copy link
Copy Markdown
Contributor Author

when I do test generation, I usually run "lnms dev:check -o myOsName" at least up to the "refreshing database ... done". That way I am sure the DB is empty and clean.

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.

3 participants