snmp_exporter icon indicating copy to clipboard operation
snmp_exporter copied to clipboard

Implement indices filters

Open sebastien-coavoux opened this issue 4 years ago • 13 comments

Hi,

This PR adds the feature of instance filtering in both the generator and the exporter. Filters in the generator are referred as "static" while filters in the exporter are "dynamic". The idea behind this, is to limit the number of oid returned by the monitored device , and in most case reduce the number of snmp get sent.

Static filters can be seen as an ease of configuration, as it make instance filtering easier to do Dynamic filters are simple for now in order to keep the exporter stateless between runs. Filter are evaluated when /snmp route is reached by prometheus by http. We could have a /filter route to make 2 separate flows but that would complexify the code.

Filters are also not made to have multiple levels. Oid are targeted by a single filter, there is no "and / or" logic at all.

However, Dynamic filter evaluation support regex. Regex matching was as complex as string comparison but with more options for the user.

Let me know what do you think. I could squash commits / remove some if necessary

sebastien-coavoux avatar Mar 01 '21 13:03 sebastien-coavoux

Nice, it will take some time to review this.

The first thing I am thinking about here is terminology. We use the term "instance" in Prometheus to refer to the target device.

I need to dig through various MIBs to see how the terminology is used there, there are some references to "INDEX", and some "instance".

From a high-level perspective, this is an "Index filter". It filters instances of results from the OID index(es). I think it would be more intuitive to align the naming in this way.

SuperQ avatar Mar 01 '21 16:03 SuperQ

Hey, any update on it? I gpg signed the commit but now the generator fails because it is not finding some OID (raritan).

Pretty strange since I have touched the mib dir neither the generator yaml.

sebastien-coavoux avatar Apr 21 '21 01:04 sebastien-coavoux

The generator thing happens when upstream MIBs cause issues.

https://github.com/prometheus/snmp_exporter/pull/647 should fix that up.

SuperQ avatar Apr 21 '21 08:04 SuperQ

Hi,

I've rebased the MR on main so that the CI is green. I've also changed the vocab from instance to index/indices ( I had to choose a side for the plural, feel free to argue on it :smile: )

Let me know if you would like something else.

sebastien-coavoux avatar May 30 '21 14:05 sebastien-coavoux

Been running with this for a while, over 800 switches/routers/firewalls with various filters. (Admin up, interface names and interface descriptions) No problems to report.

xkilian avatar Jun 22 '21 13:06 xkilian

Sorry for the long delay in reviewing. Would you mind rebasing this?

SuperQ avatar May 28 '22 06:05 SuperQ

Done !

sebastien-coavoux avatar Jun 02 '22 16:06 sebastien-coavoux

This seems mostly sane, one question about the input config. Otherwise I think we can add this.

SuperQ avatar Jun 21 '22 16:06 SuperQ

@SuperQ sorry for the delay, I was mostly off in summer. I've just added the index validation as part of Unmarshalling.

sebastien-coavoux avatar Sep 19 '22 14:09 sebastien-coavoux

This change is very well thought out, it worked flawlessly for my setup and accounted for all my whacky corner cases. It cut my time for stat collection down from 30s per router to 1.5!!!!! That's huge savings. Obviously it also cut my storage requirements greatly as well.

My only suggestion would be to make the example config in README.md a working one (filter targets should match oids listed in walk if I'm understanding correctly).

PrestonTaylor avatar Sep 20 '22 16:09 PrestonTaylor

@PrestonTaylor , is there anything not clear in https://github.com/prometheus/snmp_exporter/pull/624/files#diff-a715bdcf88cd771d7ae09bd01863bab314ff194ba28e984689e9040d87846db9R147 ? Readme in generator provides an example, maybe you meant the Readme at the top level of the repo ?

sebastien-coavoux avatar Sep 20 '22 23:09 sebastien-coavoux

@sebastien-coavoux My suggestion was in your readme you've linked to add the oids you have in your target section (1.3.6.1.2.1.2.2.1.4 and bsnDot11EssSsid) to the walk section on line 61 of your change and 54 of the original. This was required in my case for it to output the metrics, without it it would query them according to the debug but not output them. Very minor change that most people would figure out anyway I'm sure. Git doesn't let me add a code suggestion more than 3 lines from your edits or I would do so.

PrestonTaylor avatar Sep 21 '22 13:09 PrestonTaylor

@PrestonTaylor Ok I got it. I have added the 2 oid, the example provided is accurate with it. Thanks !

sebastien-coavoux avatar Sep 21 '22 14:09 sebastien-coavoux

@SuperQ in case you missed it :point_up:

sebastien-coavoux avatar Oct 04 '22 13:10 sebastien-coavoux

Any chance of this making it into a release? Been using it at scale without issues but would love to see it merged.

PrestonTaylor avatar Mar 02 '23 15:03 PrestonTaylor

Yes, It's on my todo list. Just a lot of change to review.

SuperQ avatar Mar 02 '23 15:03 SuperQ

Updated PR from the input. Also rebased on main

sebastien-coavoux avatar Mar 27 '23 23:03 sebastien-coavoux

Thanks @sebastien-coavoux, did you see #855?

SuperQ avatar Mar 28 '23 06:03 SuperQ

I haven't dive into it but it looks like that the panic is not coming from the code modified by the PR. Mar 20 18:56:07 myexporterserver snmp_exporter: panic: Unknown index type Bits comes from https://github.com/prometheus/snmp_exporter/blob/main/collector/collector.go#L747

The minimal case seems not to have filter to reproduce so I feel like it might be possible to reproduce it on main branch

I could have a look tonight, I guess the provided MIB in there would help reproduce.

sebastien-coavoux avatar Mar 28 '23 13:03 sebastien-coavoux

Just minor Go style comments.

I have just looked at the shape of the Go code and didn't check correctness (assuming that was done by others).

Would adding tests be feasible?

Just noticed this, added some tests. Unless there is other comment I think I am done for that PR :+1:

sebastien-coavoux avatar Mar 31 '23 04:03 sebastien-coavoux

From my understanding with this change it is now possible to use dynamic filters to only get interfaces that have ifAlias that match a RegEx but I struggle to find the correct syntax, does anyone have an example?

JDA88 avatar Jul 19 '23 09:07 JDA88

The readme should provide a good example of it. If not then maybe it is not clear enough so you could suggest a PR

From the snippet of readme we have

      dynamic: # dynamic filters are handed by the snmp exporter. The generator will simply pass on the configuration in the snmp.yml.
               # The exporter will do a snmp walk of the oid and will restrict snmp walk made on the targets
               # to the index matching the value in the values list.
               # This would be typically used to specify a filter for interfaces with a certain name in ifAlias, ifSpeed or admin status.
               # For example, only get interfaces that a gig and faster, or get interfaces that are named Up or interfaces that are admin Up
        - oid: 1.3.6.1.2.1.2.2.1.7
          targets:
            - "1.3.6.1.2.1.2.2.1.4"
          values: ["1", "2"]

This means that the exporter will walk 1.3.6.1.2.1.2.2.1.7 and for each index of this oid, if the values match "1" or "2" it will remember this index and do a snmp get of this index on the target .

example : 1.3.6.1.2.1.2.2.1.7.1 => 111 . The exporter will get 1.3.6.1.2.1.2.2.1.4.1

sebastien-coavoux avatar Jul 19 '23 22:07 sebastien-coavoux

I saw and understand the example but I fail to extrapolate It to my usercase. Let say I want the: ifAdminStatus (1.3.6.1.2.1.2.2.1.7) if the ifName (1.3.6.1.2.1.31.1.1.1.1) match the regex ^SW-.* Will it translate to the following code?

      dynamic: 
        - oid: 1.3.6.1.2.1.31.1.1.1.1
          targets:
            - "1.3.6.1.2.1.2.2.1.7"
          values: ["^SW-.*"]

JDA88 avatar Jul 20 '23 06:07 JDA88

Yes this would be the right snippet of config.

If it is not working for you i would suggest to open an in issue. In the meantime things to check (if it is not working) :

  • Run exporter in Debug, you should see in the logs the rules / filter evaluated
  • Snmpwalk 1.3.6.1.2.1.31.1.1.1.1 manually and look at the output. Check if the regexp apply to that output : https://play.golang.com/p/hU-XCVNgEd5

sebastien-coavoux avatar Jul 20 '23 12:07 sebastien-coavoux