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

feature: Entity State polling #7625

Merged
merged 15 commits into from Dec 2, 2017

Conversation

Projects
None yet
5 participants
@murrant
Member

murrant commented Nov 4, 2017

Display entity state on the Inventory page.
Allows for alerting based on states.

image

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

fixes: #7621

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 4, 2017

Member

So this isn't working for me, the sensors are added and then removed immediately.

However, is this not better to be added as yaml as it seems supported and then we can have a similar flag like rfc1628_compat in the os definition to load entity-sensor for OS we need it on?

Member

laf commented Nov 4, 2017

So this isn't working for me, the sensors are added and then removed immediately.

However, is this not better to be added as yaml as it seems supported and then we can have a similar flag like rfc1628_compat in the os definition to load entity-sensor for OS we need it on?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 5, 2017

Member

Yeah, the valid was the wrong array or something. This was just an exploration to see if we wanted this.

Member

murrant commented Nov 5, 2017

Yeah, the valid was the wrong array or something. This was just an exploration to see if we wanted this.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 5, 2017

Member

I definitely say we should have it :)

Member

laf commented Nov 5, 2017

I definitely say we should have it :)

feature: Entity State polling
Display entity state on the Inventory page.
Allows for alerting based on states.

@murrant murrant changed the title from Entity state sensors?? to feature: Entity State polling Nov 10, 2017

@shimamizu

This comment has been minimized.

Show comment
Hide comment
@shimamizu

shimamizu Nov 13, 2017

screen shot 2017-11-13 at 4 34 10 pm
This looks promising :) Example on an Arista 7508 chassis. (it does display SN's in that but I edited them out)

shimamizu commented Nov 13, 2017

screen shot 2017-11-13 at 4 34 10 pm
This looks promising :) Example on an Arista 7508 chassis. (it does display SN's in that but I edited them out)

@shimamizu

This comment has been minimized.

Show comment
Hide comment
@shimamizu

shimamizu Nov 13, 2017

Let me know what a simple alert rule would be to alert on say if PWR-3KT-AC-RED were to go down (aka if Power Supply in Slot 1 randomly died) based on where you're storing this in the database.

shimamizu commented Nov 13, 2017

Let me know what a simple alert rule would be to alert on say if PWR-3KT-AC-RED were to go down (aka if Power Supply in Slot 1 randomly died) based on where you're storing this in the database.

murrant added some commits Nov 14, 2017

do not display unavailable alarms (80)
add tooltip with state value

@murrant murrant added the Schema label Nov 14, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member

One big issue :(

image

Ignore the earlier data but the clean data from this PR is from 1pm onwards until it drops when I switch back from master. A change from 250 seconds to 50 seconds. I fear this will kill some devices :(

Member

laf commented Nov 15, 2017

One big issue :(

image

Ignore the earlier data but the clean data from this PR is from 1pm onwards until it drops when I switch back from master. A change from 250 seconds to 50 seconds. I fear this will kill some devices :(

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 15, 2017

Member

@laf yeah, any ideas on how to make that faster? Like I said earlier, there is a lot of data here.
5 points of data for every piece of equipment in the entitymib (which includes ports usually)

Member

murrant commented Nov 15, 2017

@laf yeah, any ideas on how to make that faster? Like I said earlier, there is a lot of data here.
5 points of data for every piece of equipment in the entitymib (which includes ports usually)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member

@laf yeah, any ideas on how to make that faster? Like I said earlier, there is a lot of data here.
5 points of data for every piece of equipment in the entitymib (which includes ports usually)

Just do it how the current sensors table does it, multi gets until we've iterated through all the data. That means we need to either store the OIDs or be able to build them from the data we do store. If it's the same 5 OIDs with the index then you probably store that now.

Member

laf commented Nov 15, 2017

@laf yeah, any ideas on how to make that faster? Like I said earlier, there is a lot of data here.
5 points of data for every piece of equipment in the entitymib (which includes ports usually)

Just do it how the current sensors table does it, multi gets until we've iterated through all the data. That means we need to either store the OIDs or be able to build them from the data we do store. If it's the same 5 OIDs with the index then you probably store that now.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 15, 2017

Member

I would think multi-getting the same data as the walk would be less efficient.

I guess there needs to be a way to disable the states for some entities?

Member

murrant commented Nov 15, 2017

I would think multi-getting the same data as the walk would be less efficient.

I guess there needs to be a way to disable the states for some entities?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member

Some options:

  • walk entStateLastChanged, compare to the DB and for those that have changed, multi get the data you want.

  • do like we do for selected ports polling and walk entStateOper, if it's enabled multi get the data you want.

Also, is it worth storing the _prev values or are you using the StateLastChanged value to alert on or something else?

Member

laf commented Nov 15, 2017

Some options:

  • walk entStateLastChanged, compare to the DB and for those that have changed, multi get the data you want.

  • do like we do for selected ports polling and walk entStateOper, if it's enabled multi get the data you want.

Also, is it worth storing the _prev values or are you using the StateLastChanged value to alert on or something else?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 15, 2017

Member

For some reason this has stopped working for me. It's probably my local install so I'll keep digging.

Member

laf commented Nov 15, 2017

For some reason this has stopped working for me. It's probably my local install so I'll keep digging.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 16, 2017

Member

Not sure about alerting. I need to come up with an alert rule or two. I'm not sure what a failed state looks like as I don't want to make my production test device fail ;)

Member

murrant commented Nov 16, 2017

Not sure about alerting. I need to come up with an alert rule or two. I'm not sure what a failed state looks like as I don't want to make my production test device fail ;)

@kkrumm1

This comment has been minimized.

Show comment
Hide comment
@kkrumm1

kkrumm1 Nov 26, 2017

Member

any word on this?

Member

kkrumm1 commented Nov 26, 2017

any word on this?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 26, 2017

Member

Haven't gotten back to it. Need to move existing code to discovery and write a more efficient poller.

Member

murrant commented Nov 26, 2017

Haven't gotten back to it. Need to move existing code to discovery and write a more efficient poller.

@laf laf added this to the 1.35 milestone Nov 27, 2017

murrant added some commits Nov 4, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Nov 28, 2017

Member

@laf see if that is better for polling time

Member

murrant commented Nov 28, 2017

@laf see if that is better for polling time

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Nov 30, 2017

Member

It is, it still increases polling time from 35/40 seconds to 85 seconds.

I'd definitely be concerned about this for people though. That's enough to tip pollers over the 5 minute mark. What about setting this as disabled by default?

Member

laf commented Nov 30, 2017

It is, it still increases polling time from 35/40 seconds to 85 seconds.

I'd definitely be concerned about this for people though. That's enough to tip pollers over the 5 minute mark. What about setting this as disabled by default?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Dec 1, 2017

Member

40s is a lot better that 200s. Perhaps disabled by default is a good thing.

Member

murrant commented Dec 1, 2017

40s is a lot better that 200s. Perhaps disabled by default is a good thing.

murrant added some commits Dec 1, 2017

Merge remote-tracking branch 'origin/entity-state' into entity-state
# Conflicts:
#	includes/polling/entity-state.inc.php
#	sql-schema/216.sql
@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Dec 2, 2017

Member

40s is a lot better that 200s. Perhaps disabled by default is a good thing.

It is yes but it's (imho) too high for a default enable.

Member

laf commented Dec 2, 2017

40s is a lot better that 200s. Perhaps disabled by default is a good thing.

It is yes but it's (imho) too high for a default enable.

@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Dec 2, 2017

The inspection completed: 1 new issues, 4 updated code elements

scrutinizer-notifier commented Dec 2, 2017

The inspection completed: 1 new issues, 4 updated code elements

@laf laf merged commit a21a3fb into librenms:master Dec 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@murrant murrant deleted the murrant:entity-state branch Dec 4, 2017

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.