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

Add a 'session' sensor for PanOS, SRX5800 Flow Accounting #8857

Merged
merged 10 commits into from May 31, 2019

Conversation

Projects
3 participants
@TheMysteriousX
Copy link
Contributor

commented Jun 30, 2018

I couldn't find any docs on adding a new sensor type so I've kind of winged it - I may have missed a few places that it needs to be added.

As an example, I've made our SRX5800's record the flow state on its SPU's - it looks like this:
screenshot 2018-06-30 22 25 24

I had a look at adding Linux TCP Established/Max, but it seems there's no maximum configured so it's of questionable use - can throw it in though if wanted, looks like:

screenshot 2018-07-01 01 03 35

TheMysteriousX added some commits Jun 30, 2018

feature: Add a 'session' sensor type
This allows things like the SRX5800 SPU's to be graphed as they expose current and maximum usage in a table.
The intent is to contain things like TTY sessions, flow counters from other vendors, or other resources with support a finite number of simultaneous users or connections.
Include the node description in the descr field
Chassis clusters use the same FPC's

@TheMysteriousX TheMysteriousX changed the title Add a 'session' sensor type and SRX5800 Flow Accounting Add a 'session' sensor type and PanOS, SRX5800 Flow Accounting Jun 30, 2018

@laf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

So for me on this one is that these are in the wrong place. Sensors are for health information which these don't strike me as being applicable for.

Someone has attempted a PR to support a generic 'sensor' based system but I don't think it's ready yet. Ideally what we should have is one central place where you can register any type of 'sensors' against but say what section they belong in.

That needs time from someone though to get that in and make it work.

@TheMysteriousX

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2018

Mempools and CPU are implemented using the same code, so that's why I implemented it as health - being as the flow table is of a finite size and can run out.

TheMysteriousX added some commits Jul 1, 2018

@TheMysteriousX TheMysteriousX force-pushed the TheMysteriousX:sessions-sensor branch from 41f5c38 to e0149c1 Jul 1, 2018

@laf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

Mempools and CPU are implemented using the same code, so that's why I implemented it as health - being as the flow table is of a finite size and can run out.

Actually mempools and cpu have there own tables but that's for a good reason (well at atleast mempools) as it stores more than just a current value + they run there own disco/poller code.

Personally I'd like to see a more generic table that registers various health metrics and records the group it belongs to rather than trying to fit other things into the sensors table.

@laf

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

You don't need to close this, my responses are my personal opinion and not of the overall project. Someone else might disagree with me and want this merging.

@TheMysteriousX TheMysteriousX reopened this Jul 2, 2018

@murrant

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

I'd be fine with this as a stop-gap. At least we would have test data. I do agree with what @laf said about needing a better place for all of it.

@murrant murrant added this to Pending in Count Sensor via automation Jan 5, 2019

@murrant murrant added the Device 🖥 label Jan 5, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

After #9606 is merged, we'll merge in one of the count sensor PRs.

@murrant

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

@TheMysteriousX I've updated this to use the count sensor, but you'll need to add test data for the sensors.

murrant added some commits Apr 29, 2019

@murrant murrant changed the title Add a 'session' sensor type and PanOS, SRX5800 Flow Accounting Add a 'session' sensor for PanOS, SRX5800 Flow Accounting May 31, 2019

@murrant murrant merged commit 75b67f2 into librenms:master May 31, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.