Multi index

[brian-brazil]

Fixes #227 #180

[RichiH]

Wait, what, w00t!

[RichiH]

I have high interest in this, but no time for anything much. I opened an internal ticket for a co-worker to test and play. Sent by mobile; please excuse my brevity.

…

On Tue, Sep 25, 2018, 17:47 Richard Hartmann ***@***.***> wrote: Wait, what, w00t! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuEI1sAAPSg26SktAuvaxkatDd0nucuks5uelAegaJpZM4W4tJu> .

[RichiH]

The default in this option should be safe as well. Sent by mobile; please excuse my brevity.

…

On Thu, Oct 4, 2018, 14:57 Brian Brazil ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In generator/README.md <#339 (comment)> : > # If the index of a table is bsnDot11EssIndex, usually that'd be the label # on the resulting metrics from that table. Instead, use the index to # lookup the bsnDot11EssSsid table entry and create a bsnDot11EssSsid label # with that value. - old_indexes: [bsnDot11EssIndex] new_index: bsnDot11EssSsid + keep_old: false # If true, don't delete old index label for this lookup. The default is not to have any lookups, which is perfectly safe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuEIy7vFiMeiO5TJtTOO7u2GSp-yrWCks5uhgXCgaJpZM4W4tJu> .

[brian-brazil]

The default discourages users from having surplus labels, and it was agreed previously that use of such a thing should only be on a case-by-case basis - which you are proposing is not in line with.

[dannyk81]

Apologies for barging in 😄

But I came across this, while looking for a way to add both the ifDescr and ifAlias labels to the various if* metrics from Network devices, currently we are replacing ifIndex with ifDescr, but I would also like to have the ifAlias as a label (indeed we solve this today with group modifiers, but that really complicates things for use when querying).

This change looks interesting, but I don't think it's going to provide a solution for this particular use case, right?

[SuperQ]

@dannyk81 Yes! This will do exactly that. We're currently arguing over the final detail about having add or remove be the default behavior.

For the record, I've been using this patch to have this behavior in my configurations. Once this is merged, the patch will not be necessary.

[RichiH]

FYI, we are currently deadlocked on the last open question; before that's resolved, I don't think we can merge this PR.

• @brian-brazil strongly believes ifIndex should be dropped by default
• I strongly believe ifIndex must be kept by default

[dannyk81]

Ohhh! @SuperQ great news! I guess I misunderstood how this works - apologies for the noise and hope to see this soon 🙏

But to be honest, I'm still not quite sure how the lookup should be defined with this to achieve it (wanted to take this branch for a spin):

    lookups:
    - source_indexes: [ifIndex, ifAlias]
      lookup: ifDescr
      keep_source_indexes: true

?

The result I'm looking for:

ifHCInOctets{ifIndex="<index>",ifDescr="EthernetX/Y",ifAlias="# <some text here> #",instance="<some cisco switch>",job="cisconxos-snmp-exporter"}

My 2 cents on your debate (though I don't presume it will have too much weight on your final decision 😅) is that the ifIndex should be kept around, since for me it provides a unique ID of the resource the metric represents and removing it by default seems not the best approach.

Having said that, I'm perfectly fine tuning this parameter to true myself if it means we get this thing merged and released 👍

I agree with @brian-brazil that snmp.yml should not be generated/tweaked manually and we as users try to abide by the rule/best-practice, however the reality is that we have cases where this is unavoidable, my example is the CheckPoint FW devices.

  - name: fwDropPcktsOut
    oid: 1.3.6.1.4.1.2620.1.1.25.5.1.10
    type: counter
    help: ' - 1.3.6.1.4.1.2620.1.1.25.5.1.10'
    indexes:
    - labelname: fwIfIndex
      type: gauge
    - labelname: fwIfDummyIndex
      type: gauge
    lookups:
    - labels:
      - fwIfIndex
      - fwIfDummyIndex
      labelname: fwIfName
      oid: 1.3.6.1.4.1.2620.1.1.25.5.1.2
      type: DisplayString

We modify the generated snmp.yml as described above, since the results returned from the device are incorrect and not supported by the generator (the solution was provided by Brian and Ben --> https://groups.google.com/forum/#!msg/prometheus-users/NDyO8qRlXoM/mUculDc-BQAJ)

I believe this is not a unique case, and SNMP is full of such examples, so the point is that we need to find a balance between keeping things clean and standardized on the one hand and on the other hand provide the users (us) with tools to handle the abnormal cases.

/end of rant ☮️

[SuperQ]

For ifIndex you need a configuration like this when using this patch:

    lookups:
    - source_indexes: [ifIndex]
      lookup: ifDescr
      keep_source_indexes: true
    - source_indexes: [ifIndex]
      lookup: ifAlias
      keep_source_indexes: true
    - source_indexes: [ifIndex]
      lookup: ifName
      keep_source_indexes: true

The explanation here is that the "source index" is the index on the MIB entry.

You can see that in the MIB:

ifEntry OBJECT-TYPE
    SYNTAX      IfEntry
    MAX-ACCESS  not-accessible
    STATUS      current
    DESCRIPTION
            "An entry containing management information applicable to a
            particular interface."
    INDEX   { ifIndex }
    ::= { ifTable 1 }

The big reason for this change is we wanted to be able to support objects that have multi-dimension indexes. For example, here's one from a wifi controller:

   wlsxWlanAPBssidEntry OBJECT-TYPE
      SYNTAX       WlanAPBssidEntry
      MAX-ACCESS   not-accessible       
      STATUS       current
      DESCRIPTION
             "BSSID Entry"
      INDEX {wlanAPMacAddress, wlanAPRadioNumber, wlanAPBSSID}
      ::= { wlsxWlanAPBssidTable 1 }

[dannyk81]

Ahhh, I see! @SuperQ, that's clear now. so the ifIndex lookup can be specified several times (each with a different lookup), I wasn't sure about that.

Thanks, this is clear now.

[dannyk81]

Hope to see this soon 🙏

[fjaeckel]

I'd love to see this merged, whats the holdup? :)

[SuperQ]

@fjaeckel We're stuck on agreeing on the default of keep_source_indexes.

[fjaeckel]

@SuperQ as long as we're documenting the default, whatever it may be, and its implications - we should be able to just roll with it. I think for myself ifIndex, if thats the only label, should be kept by default.

[SuperQ]

That's the issue. If you use a lookup, for ifDescr, ifIndex is dropped with the current default of false.

[fjaeckel]

That's the issue. If you use a lookup, for ifDescr, ifIndex is dropped with the current default of false.

I don't see it as a blocker, it can be overridden and should just be explained why some think its a good default.
I see a bigger issue in not merging this PR than setting a sane default, people maintain their own patch sets for snmp_exporter (and generator) - I for one do.

[dannyk81]

@SuperQ @RichiH @brian-brazil

Sorry for @ing like this, but is there any chance we can have closure on this? I desperately need this functionality and I've been avoiding using a private build of this PR until now... but seems like we're at an impasse 😞

I completely agree with @fjaeckel, as long as we have the option to change the default behavior, it should be a reasonable approach for everyone.

Please 🙏

[RichiH]

I called a vote to unblock this either way. I can't stress enough that while I disagree with Brian here, I sincerely thank him for doing this.

https://groups.google.com/forum/#!topic/prometheus-developers/0eMRmiIHcsM

[SuperQ]

Can you please rebase this against master?

[brian-brazil]

This is updated. Do we know which if any of the existing lookup examples are non-unique?

[SuperQ]

It's not the lookups that cause issues, it's the devices when you use them. I know HP devices will generate duplicate ifDescr="HP ProCurve Switch software loopback interface" labels when you have management IPs configured on more than one vlan. The only way to have ifDescr is by having both ifIndex and ifDescr.

[brian-brazil]

ifInterfaces I've already covered, it's the others I'm wondering about.

[SuperQ]

LGTM

[SuperQ]

I think that should cover it for now.

[SuperQ]

Looks like there's a minor difference to some metrics in the snmp.yml.

[brian-brazil]

Odd, yet another run fixed it.

[RichiH]

LGTM. Ideally, I would love to have a release right after merging this.

[brian-brazil]

Yes, this requires an immediate release due to the nature of the changes.

[RichiH]

Just to make sure: @brian-brazil is this ready to merge from your end?

[brian-brazil]

Yes, I'll do it when I'm ready to release.