aristanetworks / goarista

Fairly general building blocks used in Arista Go code and open-sourced for the benefit of all.
Apache License 2.0
206 stars 66 forks source link

ocprometheus not able to parse complex path #51

Closed topine closed 4 years ago

topine commented 4 years ago

Hello,

I`m trying to collect LLDP data using the telemetry paths ( that I found in CVP ) :

/Sysdb/l2discovery/lldp/status/local/1/portStatus/Ethernet1/1/remoteSystem/2 : sysName : { "value": "NeighborHostname" }

Exporter config :

Metrics:
   - name: lldpNeighborInfo
     path: /Sysdb/l2discovery/lldp/status/local/(?P<localIndex>.+)/portStatus/(?P<intf>.+)/remoteSystem/(?P<remoteSystemIndex>.+)/sysName/value
     help: LLDP metric info
     valuelabel: neighborName
     defaultvalue: 1

The metric is not mapped and no error logs are found.

I`m trying other paths to debug and when I try the config below :

       - name: lldpPortStatus
          path: /Sysdb/l2discovery/lldp/status/local/1/portStatus/(?P<intf>.+)/(?P<type>.+)
          help: Test with PortStatus
          valuelabel: textValue

Seems the path changes for the remotesystem, adding "_counts" after the interface :

lldpPortStatus{intf="Ethernet10\/3/_counts",job="poc-arista",textValue="textValue",type="remoteSystem"} EOS : 4.22.4M

Could you please verify?

Thank you, Anderson Topine

topine commented 4 years ago

@noredistribution @fhibler

noredistribution commented 4 years ago

Hi Anderson,

Thanks for reaching out! You see that _counts in the logs because it's a valid path (you'll see other similar Ignoring unmatched update path ... messages if you enable verbosity with -v=9), but since we don't match that, the update is ignored (the update is sent for all subpaths as you probably specified the subscription based on the parent path, ie /Sysdb/l2discovery/lldp, so that's expected) Anyways, currently you can't match the sysName value as the string is in a nominal, which is not handled in the code yet. Not sure when this will be added though

topine commented 4 years ago

Hello @noredistribution ,

Thank you for the info.

Would it work if I change the mapping to get the sysMap as a regex?

   - name: lldpNeighborInfo
     path: /Sysdb/l2discovery/lldp/status/local/(?P<localIndex>.+)/portStatus/(?P<intf>.+)/remoteSystem/(?P<remoteSystemIndex>.+)/(sysName)/value
     help: LLDP metric info
     valuelabel: neighborName
     defaultvalue: 1

image

This kind of label would be really appreciated for us, would you be open to code contribution?

noredistribution commented 4 years ago

Hi @topine,

It won't work without adding functionality to the code. I was discussing this with colleague and seems like it was an easy fix. Ran some tests and seems to work fine, we'll do a commit and merge once it gets reviewed! To answer your question, contributions are always welcome by anyone!

Thanks!

topine commented 4 years ago

Thanks a lot @noredistribution !

topine commented 4 years ago

Hello @noredistribution

Until you dont release and we need the change, I`ve forked the repo, debugged a bit and added also the feature, updated the unit tests and tested in the device, everything is working fine:

https://github.com/topine/goarista/commit/e6adc1476f2ce9c1ce4b7e4cb62239890739e7b6

Do you have an expected date for the commit? Just to have an idea until when we will need to maintain the fork.

I did not propose this as a Pull Request because you said its already implemented in your side.

Thanks a lot

noredistribution commented 4 years ago

@topine this was merged today: https://github.com/aristanetworks/goarista/commit/fd197cf57d96fbffe525eaa10ddaa485005b0a51

Feel free to propose PRs in the future!