Collect Juniper dom threshold values #2513
Conversation
stveit
changed the title
makes it so the graphite metric paths have generic names, instead of names specific to the juniper columns. Any sensor threshold will have the same naming, no matter what type of sensor it is. all you need to know is the netbox, the sensor and the type of threshold you want (high alarm, low alarm etc)
stveit
force-pushed
the
feature.juniper-dom-threshold-values
branch
from
46d2a77 to
ebc385d
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## master #2513 +/- ##
==========================================
+ Coverage 53.31% 53.80% +0.49%
==========================================
Files 554 558 +4
Lines 40315 40624 +309
==========================================
+ Hits 21494 21859 +365
+ Misses 18821 18765 -56
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
I like much of what you've done here, but I do have some reservations about the overall solution.
First of all, the statsensors plugin is built to be very generic in what it does: It collects the sensor OIDs that have been registered in the database, period. Adding MIB mapping for threshold collection to the very same plugin introduces a new kind of complexity to the plugin that I don't care too much for.
In principle, a threshold value is just "another sensor" when it comes to collection. NAV also doesn't care that the value is a "threshold" - that interpretation is up to the user who is building their Graphite expressions for threshold rules.
The special case for threshold sensors is this: They have a relationship to another sensor (and, of course, their value is likely to be a constant - but this is not important for the implementation).
Sensor objects in NAV can have an optional relationship to an interface. I'm thinking I would like to see a solution in where a Sensor can have an optional relationship to another Sensor. Maybe something like Sensor.threshold_for which would be a relationship to the sensor that this sensor is a threshold value for.
What do you think?
| for sensor in sensors: | ||
| self._logger.info(f"Internal names: {sensor.get('internal_name', None)}") |
There was a problem hiding this comment.
- This looks more like debug-level stuff, not info-level (not very interesting to a "common" user)
- Best practice for Python loggers is to use regular string formatting and expansion arguments. This ensure that the formatted string is only built if the log statement matches the currently configured log level (if debug arguments are expensive to format to strings, it doesn't happen unless DEBUG level logging is actually enabled)
- No need to specify a fallback argument of
Nonewhen using.get()on a dict -Noneis already the default fallback value.
| for sensor in sensors: | |
| self._logger.info(f"Internal names: {sensor.get('internal_name', None)}") | |
| for sensor in sensors: | |
| self._logger.debug("Internal names: %s", sensor.get('internal_name')) |
| @defer.inlineCallbacks | ||
| def _handle_thresholds(self, sensors, netboxes): | ||
| metrics = yield self._collect_thresholds(netboxes, sensors) | ||
| self._logger.info(f"Metrics: {metrics}") |
There was a problem hiding this comment.
Same comment about debug logging and argument expansion applies here.
| threshold_alert_type = models.IntegerField( | ||
| db_column='threshold_alert_type', choices=ALERT_TYPE_CHOICES, null=True |
There was a problem hiding this comment.
Reuses pre-existing alert types. Figured this could give some consistency, as the already existing "Warning" and "Alert" match quite well with the "Warning" and "Alarm" naming scheme from the juniper thresholds.
| threshold_alert_type = models.IntegerField( | ||
| db_column='threshold_alert_type', choices=ALERT_TYPE_CHOICES, null=True | ||
| ) | ||
| threshold_for_oid = VarcharField(db_column='threshold_for_oid', null=True) |
There was a problem hiding this comment.
This would make sense to be a ForeignKey to the Sensor object this is a threshold for, but it didnt seem very easy to do. The values for the sensor object that is to be created are set after discovery, but before any objects are actually made, so refering to a sensor object that doesnt exist yet seems difficult. But pairing this threshold_for_oid field with a property function threshold_for that gives the actual sensor seemed like the next best thing.
|
Kudos, SonarCloud Quality Gate passed!
|
Fixes #2340
Collects threshold values as sensors. Adds new fields to the Sensor class used to define the properties of the threshold.
Adds fields for stating if the threshold should trigger for the value being higher or lower than the given threshold, and
if the severity of the alarm that should be made if the threshold is breached.
Adds properties for getting the sensor another sensor is a threshold for, and the other way around for getting all the sensors that are thresholds for a given sensor.
draft because commit log needs fixing