Discovery - Refactor num_oid computing (#12576)

* Test the num_oid generation

* Refactor num_oid discovery

* cnt

* Cache the value to avoid unnecessary snmptranslate

* desc of function

* typo

* Negative caching as well

* Negative caching

* style

* Str::before

* Str::before

* Str::before

* Fix key,  and model Devices

* types

* types

* doc update

* re-run tests

* Use Cache class

* Simplify caching and do it into oidToNumeric

* Simplify caching and do it into oidToNumeric

* style

* Extend to 3 hours

* and not ->cache_time

* force re-run tests

* only one OID for num_oid test

* rerun tests

* typo in MD

* restore

* Exemple

* Exemple

* light change

* test

* test

* test

* back to PR

* phpstan + suggestions from Tony

* fix firebrick num_oid

Co-authored-by: Tony Murray <murraytony@gmail.com>

LibreNMS/Device/YamlDiscovery.php

@@ -24,6 +24,7 @@
 
 namespace LibreNMS\Device;
 
+use Cache;
 use Illuminate\Support\Arr;
 use Illuminate\Support\Str;
 use LibreNMS\Exceptions\InvalidOidException;
@@ -32,6 +33,8 @@ use LibreNMS\OS;
 
 class YamlDiscovery
 {
+    private static $cache_time = 1800; // 30 min, Used for oid translation cache
+
     /**
      * @param OS $os
      * @param DiscoveryItem|string $class
@@ -85,16 +88,7 @@ class YamlDiscovery
                     // determine numeric oid automatically if not specified
                     if (! isset($data['num_oid'])) {
                         try {
-                            d_echo('Info: Trying to find a numerical OID for ' . $data['value'] . '.');
+                            $data['num_oid'] = static::computeNumericalOID($device, $data);
-                            $search_mib = $device['dynamic_discovery']['mib'];
-                            if (Str::contains($data['oid'], '::') && ! (Str::contains($data['value'], '::'))) {
-                                // We should search this mib first
-                                $exp_oid = explode('::', $data['oid']);
-                                $search_mib = $exp_oid[0] . ':' . $search_mib;
-                            }
-                            $num_oid = static::oidToNumeric($data['value'], $device, $search_mib, $device['mib_dir']);
-                            $data['num_oid'] = $num_oid . '.{{ $index }}';
-                            d_echo('Info: We found numerical oid for ' . $data['value'] . ': ' . $data['num_oid']);
                         } catch (\Exception $e) {
                             d_echo('Error: We cannot find a numerical OID for ' . $data['value'] . '. Skipping this one...');
                             continue;
@@ -129,6 +123,32 @@ class YamlDiscovery
         return $items;
     }
 
+    /**
+     * @param array $device Device we are working on
+     * @param array $data Array derived from YAML
+     * @return string
+     */
+    public static function computeNumericalOID($device, $data)
+    {
+        d_echo('Info: Trying to find a numerical OID for ' . $data['value'] . '.');
+        $search_mib = $device['dynamic_discovery']['mib'];
+        $mib_prefix_data_oid = Str::before($data['oid'], '::');
+        if (! empty($mib_prefix_data_oid) && empty(Str::before($data['value'], '::'))) {
+            // We should search value in this mib first, as it is explicitely specified
+            $search_mib = $mib_prefix_data_oid . ':' . $search_mib;
+        }
+
+        try {
+            $num_oid = static::oidToNumeric($data['value'], $device, $search_mib, $device['mib_dir']);
+        } catch (\Exception $e) {
+            throw $e;
+        }
+
+        d_echo('Info: We found numerical oid for ' . $data['value'] . ': ' . $num_oid);
+
+        return $num_oid . '.{{ $index }}';
+    }
+
     /**
      * @param string $name Name of the field in yaml
      * @param string $index index in the snmp table
@@ -192,7 +212,7 @@ class YamlDiscovery
             $name = $discovery_data[$name];
         }
 
-        if (isset($pre_cache[$discovery_data['oid']][$index][$name])) {
+        if (! is_array($discovery_data['oid']) && isset($pre_cache[$discovery_data['oid']][$index]) && isset($pre_cache[$discovery_data['oid']][$index][$name])) {
             return $pre_cache[$discovery_data['oid']][$index][$name];
         }
 
@@ -356,13 +376,21 @@ class YamlDiscovery
         if (self::oidIsNumeric($oid)) {
             return $oid;
         }
-
+        $key = 'YamlDiscovery:oidToNumeric:' . $mibdir . '/' . $mib . '/' . $oid;
-        foreach (explode(':', $mib) as $mib_name) {
+        if (Cache::has($key)) {
-            if ($numeric_oid = snmp_translate($oid, $mib_name, $mibdir, null, $device)) {
+            $numeric_oid = Cache::get($key);
-                break;
+        } else {
+            foreach (explode(':', $mib) as $mib_name) {
+                $numeric_oid = snmp_translate($oid, $mib_name, $mibdir, null, $device);
+                if ($numeric_oid) {
+                    break;
+                }
             }
         }
 
+        //Store the value
+        Cache::put($key, $numeric_oid, self::$cache_time);
+
         if (empty($numeric_oid)) {
             throw new InvalidOidException("Unable to translate oid $oid");
         }

doc/Developing/os/Health-Information.md

@@ -88,8 +88,9 @@ are as follows:
 - `oid` (required): This is the name of the table you want to do the snmp walk on.
 - `value` (optional): This is the key within the table that contains
   the value. If not provided willuse `oid`
-- `num_oid` (optional): If not provided, this parameter should be computed
+- `num_oid` (required for PullRequests): If not provided, this parameter should be computed
-  automatically by discovery process. This is the numerical OID that contains
+  automatically by discovery process. This parameter is still required to
+  submit a pull request. This is the numerical OID that contains
   `value`. This should usually include `{{ $index }}`.
   In case the index is a string, `{{ $index_string }}` can be used instead.
 - `divisor` (optional): This is the divisor to use against the returned `value`.
@@ -163,13 +164,13 @@ the variable name.  Example `{{ $ifName:2 }}`
                       value: 1
 ```
 
-> ``` op ``` can be any of the following operators :
+`op` can be any of the following operators :
->
+
 > =, !=, ==, !==, <=, >=, <, >,
 > starts, ends, contains, regex, in_array, not_starts,
 > not_ends, not_contains, not_regex, not_in_array, exists
->
+
-> Example:
+Example:
 
 ```yaml
                     skip_values:
@@ -187,6 +188,21 @@ the variable name.  Example `{{ $ifName:2 }}`
                       value: false
 ```
 
+```yaml
+        temperature:
+            data:
+                -
+                    oid: hwOpticalModuleInfoTable
+                    value: hwEntityOpticalTemperature
+                    descr: '{{ $entPhysicalName }}'
+                    index: '{{ $index }}'
+                    skip_values:
+                        -
+                            oid: hwEntityOpticalMode
+                            op: '='
+                            value: '1'
+```
+
 If you aren't able to use yaml to perform the sensor discovery, you
 will most likely need to use Advanced health discovery.
 

includes/definitions/discovery/firebrick.yaml

@@ -14,6 +14,7 @@ modules:
                 -
                     oid: fbMonitoringMib
                     value: fbMonReadingValue
+                    num_oid: '.1.3.6.1.4.1.24693.100.1.1.1.4.{{ $index }}'
                     descr: 'fbMonReadingName'
                     index: 'fbMonReadingValue.{{ $index }}'
                     low_limit: 1000

includes/discovery/functions.inc.php

@@ -886,9 +886,9 @@ function discovery_process(&$valid, $device, $sensor_class, $pre_cache)
                     $user_function = $data['user_func'];
                 }
                 // get the value for this sensor, check 'value' and 'oid', if state string, translate to a number
-                $data_name = isset($data['value']) ? $data['value'] : $data['oid'];  // fallback to oid if value is not set
+                $data['value'] = isset($data['value']) ? $data['value'] : $data['oid'];  // fallback to oid if value is not set
 
-                $snmp_value = $snmp_data[$data_name];
+                $snmp_value = $snmp_data[$data['value']];
                 if (! is_numeric($snmp_value)) {
                     if ($sensor_class === 'temperature') {
                         // For temp sensors, try and detect fahrenheit values
@@ -917,20 +917,11 @@ function discovery_process(&$valid, $device, $sensor_class, $pre_cache)
                 // Check if we have a "num_oid" value. If not, we'll try to compute it from textual OIDs with snmptranslate.
                 if (empty($data['num_oid'])) {
                     try {
-                        d_echo('Info: Trying to find a numerical OID for ' . $data_name . '.');
+                        $data['num_oid'] = YamlDiscovery::computeNumericalOID($device, $data);
-                        $search_mib = $device['dynamic_discovery']['mib'];
-                        if (Str::contains($data['oid'], '::') && ! (Str::contains($data_name, '::'))) {
-                            // We should search this mib first
-                            $exp_oid = explode('::', $data['oid']);
-                            $search_mib = $exp_oid[0] . ':' . $search_mib;
-                        }
-                        $num_oid = YamlDiscovery::oidToNumeric($data_name, $device, $search_mib, $device['mib_dir']);
-                        $data['num_oid'] = $num_oid . '.{{ $index }}';
-                        d_echo('Info: We found numerical oid for ' . $data_name . ': ' . $data['num_oid']);
                     } catch (\Exception $e) {
-                        d_echo('Error: We cannot find a numerical OID for ' . $data_name . '. Skipping this one...');
+                        d_echo('Error: We cannot find a numerical OID for ' . $data['value'] . '. Skipping this one...');
                         $skippedFromYaml = true;
-                        // Cause we still don't have a num_oid
+                        // Because we don't have a num_oid, we have no way to add this sensor.
                     }
                 }
 

misc/discovery_schema.json

@@ -170,7 +170,8 @@
                                 },
                                 "additionalProperties": false,
                                 "required": [
-                                    "oid"
+                                    "oid",
+                                    "num_oid"
                                 ]
                             }
                         }
@@ -278,6 +279,7 @@
                                         "additionalProperties": false,
                                         "required": [
                                             "descr",
+                                            "num_oid",
                                             "oid",
                                             "states"
                                         ]
@@ -444,6 +446,7 @@
                         "additionalProperties": false,
                         "required": [
                             "descr",
+                            "num_oid",
                             "oid"
                         ]
                     }