Enhance sFlow coverage in netflow module

[akarneliuk]

Use Case

Telegraf input pluginnetflow, which is now recommended for usage if I need sflow by the team, relies on goflow2 library. That library already does extensive parsing of the extended_gateway attribute, whilst only a subset of these fields are exposed in telegraf:

1. inputs.netflow plugin parses only next_hop, src_as, peer_as, prev_as
2. goflow2 already supports more than 10 attribures

The use case is to expose all those attributes in Telegraf, so that they could be used

Expected behavior

All sflow attributes, which are parsed by goflow2 are exposed to sflow. Specifically, all extended_gateway.

Actual behavior

Currently only a subset of attributes are exposed , and an important attributes such as as-path and communities aren't available.

Additional info

No response

[powersj]

Hi,

important attributes such as as-path and communities aren't available.

These five are already covered:

NextHop           utils.IPAddress `json:"next-hop"`
SrcAS             uint32          `json:"src-as"`
ASDestinations    uint32          `json:"as-destinations"`
SrcPeerAS         uint32          `json:"src-peer-as"`
ASPath            []uint32        `json:"as-path"`

I am calling this out since you said as-path isn't available, when it is included today. If it is present, see here.

That appears to leave these 7:

NextHopIPVersion  uint32          `json:"next-hop-ip-version"`
AS                uint32          `json:"as"`
ASPathType        uint32          `json:"as-path-type"`
ASPathLength      uint32          `json:"as-path-length"`
CommunitiesLength uint32          `json:"communities-length"`
Communities       []uint32        `json:"communities"`
LocalPref         uint32          `json:"local-pref"`

I'm not sure how @srebhan chose what to include, but I'm going to have him comment on this when he is back. The two *Length items probably does not make sense to include, nor the IP version? It seems this really does come down to adding Communities?

[akarneliuk]

Could we not implement all of them? Are there any disadvantages of having all fields of the struct exposed?

[powersj]

Could we not implement all of them? Are there any disadvantages of having all fields of the struct exposed?

In general, we like to expose metrics that are actually useful to a user. Not act as a pass through for absolutely everything.

[akarneliuk]

But in this case you limit user to what you believe user need, which is in this case is a subset of what standard sflow v5 implements.

[powersj]

Then you could help educate both us and others who come along and explain why some of those fields are useful. As I called out above, how is length ever useful?

To be clear, I did not say no to anything, but said it doesn't make sense to export useless fields.

[srebhan]

@akarneliuk and @powersj my initial thought was to include what is also available in netflow or IPFIX metrics, but I don't see an issue adding additional fields. What I want though is to also add an option to tag the "message type" to the metric as currently there are a lot of different metrics output with the same "series key"...

[akarneliuk]

@powersj I get your point. My point is: give users everything what protocol has and let users to figure out what is useful for them. Now, I do get that with the variety of plugins Telegraf supports it could be very overwhelming to support all features of all plugins, so I totally understand to be driven by asks, what allows to focus what users need :+1:

@srebhan , I think some sort of concatenation between the struct name and struct fields may be interesting. somewhat like, extended_gateway__next_hop or extended_gateway/next_hop (or even make this option configurable, which join symbol to utilise).

[srebhan]

@akarneliuk making this configurable add too much complexity IMO. You can still rename the fields using processors...

[srebhan]

@akarneliuk please check the binary in PR #15521, available as soon as CI finished the tests, and let me know if this fixes the issue!

[akarneliuk]

Hey @srebhan, Thanks for putting that together. I will test tomorrow and will let you know. Best, Anton

[akarneliuk]

@srebhan, i have tested, thanks. It looks though this code doesn't have fix you did for drops some time before. Isbthat expected?

[srebhan]

@akarneliuk yes this is expected as we cannot do cumulative PRs because then nobody can review it. ;-) Once this PR is merged, I will rebase the "drop" PR so you get both features...

[akarneliuk]

Right, got it. Looking forward to having those both relased. Thank you so much on working on both of them.