changes cisco_fxos_sensors to base on entity_sensors (reopen PR) #363

thl-cmk · 2021-04-15T15:13:06Z

-> remove fxos files
-> split entity_sensors.py in utils/enttity_sensors.py end entity_sensors.py
-> new cisco_entity_sensors.py: register.snmp/parse function (not for undefined/temp) -> checks in entity_sensors.py
-> new test_cisco_entity_sensors.py

Known Issues:
-> Service name for power_presence "Power Sensor" overlaps with power_consumption sensors -> change to "Power supply"
-> default values lower for fan are in conflict with device status -> needs options like 'temperature'
-> some ASA devices have no valid scaling value (always 1), posible workaround:
in "parse_entity_sensors" check scaling value -> if 1 change to 9

Posible improvements:
-> WATO: hw_fan.py -> change "output_metrics" -> CheckBox to FixedValue -> on click less ;-)
-> name field in EntitySensor is not used -> can be removed

-> remove *fxos* files -> split entity_sensors.py in utils/enttity_sensors.py end entity_sensors.py -> new cisco_entity_sensors.py: register.snmp/parse function (not for undefined/temp) -> checks in entity_sensors.py -> new test_cisco_entity_sensors.py Known Issues: -> Service name for power_presence "Power Sensor" overlaps with power_consumption sensors -> change to "Power supply" -> default values lower for fan are in conflict with device status -> needs options like 'temperature' -> some ASA devices have no valid scaling value (always 1), posible workaround: in "parse_entity_sensors" check scaling value -> if 1 change to 9 Posible improvements: -> WATO: hw_fan.py -> change "output_metrics" -> CheckBox to FixedValue -> on click less ;-) -> name field in EntitySensor is not used -> can be removed

jherbel

Besides some minor issues, we should discuss the known issues you mentioned:

Service name for power_presence "Power Sensor" overlaps with power_consumption sensors -> change to "Power supply"
Where exactly does this overlap occur? Within this plugin or are there overlaps with another plugin?
default values lower for fan are in conflict with device status -> needs options like 'temperature'
Can this for the moment be overcome by changing the lower levels with a rule?
some ASA devices have no valid scaling value (always 1), posible workaround:
in "parse_entity_sensors" check scaling value -> if 1 change to 9
What value do we get from entPhySensorScale report in such cases?

jherbel · 2021-04-16T06:38:58Z

cmk/base/plugins/agent_based/entity_sensors.py

-    item: str,
-    params: TempParamType,
-    section: EntitySensorSection,
+        item: str,


Please undo these changes

jherbel · 2021-04-16T06:39:11Z

cmk/base/plugins/agent_based/entity_sensors.py

-    item: str,
-    params: Mapping[str, Any],
-    section: EntitySensorSection,
+        item: str,


Please undo these changes

jherbel · 2021-04-16T06:39:15Z

cmk/base/plugins/agent_based/entity_sensors.py

-    item: str,
-    params: Mapping[str, Any],
-    section: EntitySensorSection,
+        item: str,


Please undo these changes

jherbel · 2021-04-16T06:43:13Z

tests/unit/cmk/base/plugins/agent_based/test_cisco_entity_sensors.py

+)
+
+from cmk.base.plugins.agent_based.entity_sensors import (
+    check_entity_sensors_fan,


Any tests for entity_sensors should be implemented in test_entity_sensors.py. Here, you should only test the parse function.

Any tests for entity_sensors should be implemented in test_entity_sensors.py. Here, you should only test the parse function.

Ok, so i move the test over to test_entity_sensors.py (except for the parse section), or is there than no additional testing nessecary? And I can remove these additional tests?

You can move them over

OK. If I move the discover function tests we will still use functions from both plugins. Is this ok.
e.g. "discover_entity_sensors_power_presence" and "parse_cisco_entity_sensors" in "test_discover_entity_sensors_power_presence"

No, its better to not mix that. Just copy _section over to the other file. Also, I guess the discovery and the check function are well tested, so there is no need to add that many tests.

cmk/base/plugins/agent_based/cisco_entity_sensors.py

jherbel · 2021-04-16T07:00:03Z

cmk/base/plugins/agent_based/cisco_entity_sensors.py

+        # this MIB, thus we use OID as item instead
+
+        # do not add undefined and temp sensors (duplicate with cisco_temperature)
+        if sensor_type_nr not in ['0', '8']:


We have quite some duplicate code here. It would be better to define a generic parse function in utils/entity_sensors.py which takes an additional input argument
sensor_types_ignore: Container[str]
You can then use this function for both snmp sections, for example

def parse_entity_sensors(string_table: List[StringTable]): return utils.parse_entity_sensors(string_table, set())

thl-cmk · 2021-04-16T07:44:09Z

Besides some minor issues, we should discuss the known issues you mentioned:

Service name for power_presence "Power Sensor" overlaps with power_consumption sensors -> change to "Power supply"
Where exactly does this overlap occur? Within this plugin or are there overlaps with another plugin?
true, to a other plugin, at the moment it's a CheckPoint/Dell Server, this has a Power supply presence/function sensor and a Power consumtion sensor, so in the long run it would make sense to have a common naming over all these type of plugins. Might be better to change it now before the plugin is out with the next public release.

default values lower for fan are in conflict with device status -> needs options like 'temperature'
Can this for the moment be overcome by changing the lower levels with a rule?
it could, but it's kind of ugly

some ASA devices have no valid scaling value (always 1), posible workaround:
in "parse_entity_sensors" check scaling value -> if 1 change to 9
What value do we get from entPhySensorScale report in such cases?
The device reports 1, meaning 10**(-24)

jherbel · 2021-04-16T08:02:53Z

true, to a other plugin, at the moment it's a CheckPoint/Dell Server, this has a Power supply presence/function sensor and a Power consumtion sensor, so in the long run it would make sense to have a common naming over all these type of plugins. Might be better to change it now before the plugin is out with the next public release.

which plugin?

The device reports 1, meaning 10**(-24)

Can 1 ever be correct? Do we have a way of knowing?

thl-cmk · 2021-04-16T08:22:55Z

true, to a other plugin, at the moment it's a CheckPoint/Dell Server, this has a Power supply presence/function sensor and a Power consumtion sensor, so in the long run it would make sense to have a common naming over all these type of plugins. Might be better to change it now before the plugin is out with the next public release.

which plugin?
checkpoint_sensors, not yet public ;-)

The device reports 1, meaning 10**(-24)

Can 1 ever be correct?
for fan speed I think 1 can't never be correct or it must be a very slow fan
for temp, I think also not, at least for a sensor
Do we have a way of knowing?
I think not realy. In my option the best way would be to have some kind of option for the parse function, so this could become configurable.

From the MIB description of sensorValue: "Range: -1000000000 - 1000000000"
so a fan with sensorValue = 1000000000 (max) and sensorScale =1 (as reported) will rotate at 9.999999999999999e-16 RPM
this must be a kind of very acurate sensor to measure this little value, I think this is true for temp also

jherbel · 2021-04-16T09:48:24Z

checkpoint_sensors, not yet public ;-)

Ok, in that case, everything is fine I would say, since its not even a plugin which is part of Checkmk

From the MIB description of sensorValue: "Range: -1000000000 - 1000000000"
so a fan with sensorValue = 1000000000 (max) and sensorScale =1 (as reported) will rotate at 9.999999999999999e-16 RPM
this must be a kind of very acurate sensor to measure this little value, I think this is true for temp also

Then it would probably make sense to simply hard-code this. I.e., neither temperature nor fan sensors should ever have sensorScale =1 and if this occurs, we set sensorScale =9. However, I would suggest do this afterwards in a separate PR afterwards (small bugfix).

thl-cmk · 2021-04-16T10:25:44Z

checkpoint_sensors, not yet public ;-)

Ok, in that case, everything is fine I would say, since its not even a plugin which is part of Checkmk

This is not entirely true, these checks are already there (except for the Power consumption) but sitll in /checks.
Especially checkpoint_powersupply, where the service name is "Power Supply". Basically this is the same as the entity_sensors_power check. Just curious, why is this such a problem to straighten the names?

From the MIB description of sensorValue: "Range: -1000000000 - 1000000000"
so a fan with sensorValue = 1000000000 (max) and sensorScale =1 (as reported) will rotate at 9.999999999999999e-16 RPM
this must be a kind of very acurate sensor to measure this little value, I think this is true for temp also

Then it would probably make sense to simply hard-code this. I.e., neither temperature nor fan sensors should ever have sensorScale =1 and if this occurs, we set sensorScale =9. However, I would suggest do this afterwards in a separate PR afterwards (small bugfix).

Ok. could look like this in the parse function:

if sensor_type_nr in ["8", "10"] and scaling_nr == "1":
            scaling_nr = "9"

jherbel · 2021-04-16T10:58:23Z

This is not entirely true, these checks are already there (except for the Power consumption) but sitll in /checks.
Especially checkpoint_powersupply, where the service name is "Power Supply". Basically this is the same as the entity_sensors_power check. Just curious, why is this such a problem to straighten the names?

Ah sry, I thought you meant check plugins that you had lying around locally or something. In general, changing service descriptions is problematic, since it can affect a lot of customers (you have to maybe changes rules etc.). I don't see the overlap between checkpoint_powersupply and entity_sensors. checkpoint_powersupply has
'service_description': "Power Supply %s",
and entity_sensors_power_presence has
service_name='Power %s',
However, in entity_sensors, we always have "Sensor" in the item name (_reformat_sensor_name).

Ok. could look like this in the parse function:
if sensor_type_nr in ["8", "10"] and scaling_nr == "1":
scaling_nr = "9"

Yes, but as I said, please in a separate commit afterwards.

thl-cmk · 2021-04-16T11:11:10Z

Ah sry, I thought you meant check plugins that you had lying around locally or something.....
I understand that, on the other hand, it is very annoying when you have products from different vendors and the "basically" same thing in CMK has different functions and names. In this case, for example, I couldn't just say that all power sensors are shown globally because they have different names. The scond thing, this check is very new, meaning not a lot of custumers will have this yet. So changing it now will be more easy than later when every one is using it. But in the end your decission.

One more thing about the naming. A quick grep over your code base shows, that most of this checks use the term "Power supply" (18), some use "Powersupply" (2) or "Power supply satus" (2). and there is at least one "Power consumtion" check (fsc_sc2_power_consumption).

jherbel · 2021-04-16T11:17:25Z

Ok, what exactly do you want to rename into what?

thl-cmk · 2021-04-16T11:23:39Z

From:

    service_name='Power %s',

to:

    service_name='Power Supply %s',

jherbel · 2021-04-16T12:08:43Z

For now, we leave this as it is.

jherbel

The unit tests for the parse function can all be moved to .../utils/test_entity_sensors.py. There, you can tests cases with and without exclusion.

jherbel · 2021-04-20T06:00:02Z

cmk/base/plugins/agent_based/utils/entity_sensors.py

+    section: EntitySensorSection = {}
+    sensor_names = {i[0]: i[1] for i in string_table[0]}
+    for oid_end, sensor_type_nr, scaling_nr, reading, status_nr, device_unit in string_table[1]:
+        if sensor_type_nr not in sensor_types_ignore:


if sensor_type_nr in sensor_types_ignore: continue

this avoid indenting the whole block afterwards

jherbel · 2021-04-20T06:00:48Z

cmk/base/plugins/agent_based/utils/entity_sensors.py

+
+
+def parse_entity_sensors(string_table: List[StringTable],
+                         sensor_types_ignore: Optional[Container[str]] = None) -> EntitySensorSection:


use set() here (and typization Container[str])

thl-cmk · 2021-04-20T08:21:26Z

The unit tests for the parse function can all be moved to .../utils/test_entity_sensors.py. There, you can tests cases with and without exclusion.

You mean the last test in .../utils/test_cisco_entity_sensors.py? Even if then have a cross reference betwen entity_sensors and cisco_entity_sensors? If yes, I will then remove .../utils/test_cisco_entity_sensors.py completely.

jherbel · 2021-04-20T08:27:31Z

You mean the last test in .../utils/test_cisco_entity_sensors.py? Even if then have a cross reference between entity_sensors and cisco_entity_sensors? If yes, I will then remove .../utils/test_cisco_entity_sensors.py completely.

What I mean is that there is no need to have separate tests for parse_cisco_entity_sensors and entity_sensors.parse_entity_sensors. Simply test the parse function utils.entity_sensors.

jherbel

Looks good from my side, are there any other changes you want to do here? Otherwise Ill go ahead and merge.

thl-cmk · 2021-04-22T07:46:01Z

Hi Joerg,

I think for now we should move forward with this PR. The oher things we discussed, I guess we need to do this at a later time.

/Thomas

jherbel · 2021-04-22T08:00:17Z

Ok, then I will go ahead and merge this. Bugfixes (such as the scaling) can be handled in later PRs.

jherbel requested changes Apr 16, 2021

View reviewed changes

moves parse function to utils

62f434a

jherbel requested changes Apr 20, 2021

View reviewed changes

moves parse test function to utils/test_entity_sensors

9ea0a75

jherbel approved these changes Apr 22, 2021

View reviewed changes

LarsMichelsen closed this in 0feb551 Apr 22, 2021

thl-cmk deleted the cisco_fxos_sensors branch November 25, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes cisco_fxos_sensors to base on entity_sensors (reopen PR) #363

changes cisco_fxos_sensors to base on entity_sensors (reopen PR) #363

thl-cmk commented Apr 15, 2021

jherbel left a comment

jherbel Apr 16, 2021

jherbel Apr 16, 2021

jherbel Apr 16, 2021

jherbel Apr 16, 2021

thl-cmk Apr 16, 2021

jherbel Apr 16, 2021

thl-cmk Apr 16, 2021

jherbel Apr 19, 2021

jherbel Apr 16, 2021

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021 •

edited

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021 •

edited

jherbel commented Apr 16, 2021

jherbel left a comment

jherbel Apr 20, 2021

jherbel Apr 20, 2021

thl-cmk commented Apr 20, 2021

jherbel commented Apr 20, 2021

jherbel left a comment

thl-cmk commented Apr 22, 2021

jherbel commented Apr 22, 2021



		def parse_entity_sensors(string_table: List[StringTable],
		sensor_types_ignore: Optional[Container[str]] = None) -> EntitySensorSection:

Navigation Menu

changes cisco_fxos_sensors to base on entity_sensors (reopen PR) #363

changes cisco_fxos_sensors to base on entity_sensors (reopen PR) #363

Conversation

thl-cmk commented Apr 15, 2021

jherbel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021 • edited

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021

jherbel commented Apr 16, 2021

thl-cmk commented Apr 16, 2021 • edited

jherbel commented Apr 16, 2021

jherbel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thl-cmk commented Apr 20, 2021

jherbel commented Apr 20, 2021

jherbel left a comment

Choose a reason for hiding this comment

thl-cmk commented Apr 22, 2021

jherbel commented Apr 22, 2021

jherbel commented Apr 16, 2021 •

edited

thl-cmk commented Apr 16, 2021 •

edited