enh(network::stormshield::snmp): added and updated multiple modes - #6258
enh(network::stormshield::snmp): added and updated multiple modes#6258crsuser wants to merge 10 commits into
Conversation
c2eb12c to
5c7ea08
Compare
| my @mem_labels = qw(asq icmp frag host system dtrack socket etherstate user); | ||
|
|
||
| sub set_counters { | ||
| my ($self, %options) = @_; |
There was a problem hiding this comment.
Hello @crsuser , thanks for the contribution !
I'm in the process of integrating changes in this file to the perl plugins and our monitoring connector.
I see you removed the prefix_memory_output() function and all the nlabel from the counter, could you expand a bit on why you wanted to remove them ?
There was a problem hiding this comment.
Hello! Thanks for the quick response.
The changes to the plugin were prompted by a new version of Stormshield firewalls, which introduced new MIBs and OIDs. Specifically, all versions post 4.8.9 now fetch metrics via a table, rather than using a string containing all metrics separated by ", " as in previous versions.
Given this architectural shift, I took the opportunity to refactor the file. As part of this overhaul, I removed the prefix_memory_output() function as I considered it less relevant in the output.
| ] | ||
| } | ||
| }, | ||
| { label => 'conn', nlabel => 'memory.connections.percentage', set => { |
There was a problem hiding this comment.
you removed the conn metric from the plugin, but for user using legacy stormshield and new centreon version this will remove a data point they already have.
I see in l228 the oid snsMemConn that you translate later to "asq", is it the same concept ? if yes we can use the same name "conn" to avoid a breaking change.
There was a problem hiding this comment.
Yes, snsMemConn corresponds to the OID that we renamed ASQ in the plugin. To be more specific, it is a combination of the two : it refers to the percentage of memory usage on ASQ connections.
I renamed it to align with current SNS terminology. If you consider this as a breaking change, you can keep the conn name. It's totally fine for us !
However, when testing the commits you made today, the ASQ/Conn metric is no longer being returned.
This is because, in the perfdatas, the key_values name remained “asq” instead of “conn”.
I've submitted a review to fix this in omercier's PR.
| perfdatas => [ | ||
| { template => '%.2f', min => 0, max => 100, unit => '%' } | ||
| { | ||
| label => 'mem_host', |
There was a problem hiding this comment.
Changing our metric names is sadly considered a breaking change, so we can't add this "label" key.
If you want better perfdata name you can use --use-new-perfdata option which will enable the "nlabel", providing better metrics name
There was a problem hiding this comment.
Understood on the constraint. This was added to give clearer names, without realizing this counts as a breaking rename rather than an addition.
I saw in your commits that you're using nlabels, that works perfectly for me.
|
|
||
| $self->{maps_counters}->{global} = [ | ||
| { label => 'total', nlabel => 'memory.usage.percentage', set => { |
There was a problem hiding this comment.
why did you remove the total metric ? was the computation wrong ?
There was a problem hiding this comment.
The total metric was removed because it isn't relevant in this context. Each metric (memory percentage) is independent and can range from 0% to 100% on its own. Summing them would result in a total that could exceed 100%, which would be misleading and not provide meaningful insight.
| $mem_values{'system'} = $values[6]; | ||
|
|
||
| if ($fields == 8) { | ||
| $mem_values{'socket'} = $values[7]; |
There was a problem hiding this comment.
does the meaning of the legacy oid have changed ?
I see in the old code we expecte either 6 or 7 field, with the optionnal one on the middle, your change expect 7 or 8 of them ?
does this behaviour changed in a previous stormshield version or was this always the case ?
There was a problem hiding this comment.
The 6-7 field expectation in the old file was always wrong. It was a longstanding bug there, not a behavior change on the Stormshield side.
The correct field is 7 or 8.
3 participants
Community contributors
Description
This pull request introduces three new operational modes and significantly enhances several existing ones.
The new modes added are:
auto_update: Handles automatic update checks and procedures.ha_cluster: Provides monitoring capabilities for High Availability clusters.interfaces_disco: A dedicated discovery mode designed to automatically detect and enumerate network interfaces on the target device. This mode simplifies the configuration process by dynamically identifying available interfaces rather than requiring manual definition.router_disco: A dedicated discovery mode designed to automatically detect and aggregate gateways for Stormshield routers. This mode simplifies monitoring by dynamically identifying principal and backup paths, analyzing their latency and packet loss, and reporting a unified health status for the entire routing configuration.Existing modes have also been updated to integrate new MIBs and data points, ensuring broader and more granular supervision coverage.
Type of change
How this pull request can be tested ?
To verify the new functionality, please follow these steps:
Test
interfaces_disco(Discovery Mode):--mode=interfaces-discooption against a target device.Test
router_disco(Discovery Mode):--mode=router-discooption against a target device.Test
auto_update:--mode=auto-update.Test
ha_cluster:--mode=ha-clusteragainst a configured HA cluster.Checklist