feat(inputs.nftables): Monitor named counters#18246
feat(inputs.nftables): Monitor named counters#18246niklasf wants to merge 1 commit intoinfluxdata:masterfrom
Conversation
telegraf-tiger
bot
added
feat
niklasf
changed the title
niklasf
force-pushed
the
feat/nftables-consistency
branch
from
882d1f1 to
1929c4e
Compare
plugins/inputs/nftables/sample.conf
Outdated
| ## Include anonymous counters, identified by a unique comment on the rule. | ||
| ## Rules without a comment are ignored. | ||
| # anonymous_counters = true |
There was a problem hiding this comment.
How about making this an include field with potential values
set: additionally monitor named sets as introduced in PR feat(inputs.nftables): Monitor set element counts #18134anonymous-counters: additionally monitor anonymous counters as introduced in this PR
with an empty default?
This will make the whole construct more speaking and allows the user to configure the plugin output in a fine-grained fashion.
There was a problem hiding this comment.
Sounds good.
Regarding the default: I think the best default would be include = [ "counters", "sets" ], but omitting anonymous counters by default would be a breaking change (v1.37.0 monitors them), so I set include = [ "counters", "sets", "anonymous-counters" ].
There was a problem hiding this comment.
Well I would omit sets and anonymous-counters in the defaults, i.e.
include = [ "counters"]as this would represent what is there in v1.37.0.
There was a problem hiding this comment.
Unfortunately v1.37.0 is essentially include = [ "anonymous-counters" ], which in my opinion now looks like a weird choice, but it's just how this plugin started out. It's at least weird enough that it required that note:
Important
Rules are identified by the associated comment so those comments have to be unique! Rules without comment are ignored.
include = [ "counters" ] or include = [ "sets", "counters" ] would make more sense to me, but I believe for backwards compatibility it must then be
include = [ "counters", "anonymous-counters" ] or include = [ "sets", "counters", "anonymous-counters" ] respectively.
Thoughts?
There was a problem hiding this comment.
Well if v1.37.0 is include = [ "anonymous-counters" ] then this should be the default to reproduce v1.37.0 behavior in following releases.
niklasf
force-pushed
the
feat/nftables-consistency
branch
2 times, most recently
from
19a56cd to
a4515c9
Compare
| > [!IMPORTANT] | ||
| > Rules are identified by the associated comment so those **comments have to be | ||
| > unique**! Rules without comment are ignored. | ||
|
|
There was a problem hiding this comment.
I think this is still true, isn't it?
There was a problem hiding this comment.
Depending on what we decide for the defaults, it's either a minor detail for the configuration of anonymous-counters or a very important point. Will restore or improve wording in the sample.conf, depending on that choice.
| default: | ||
| return fmt.Errorf("unknown include: %s", include) | ||
| } | ||
| } |
There was a problem hiding this comment.
How about looping over the includes in gather like (pseudo-code)
for _, include := range n.Include {
switch include {
case "counters":
if err := n.gatherNamedCounters(acc, nftables); err != nil {
return fmt.Errorf("gathering named counters failed: %w", err)
}
case "sets":
if err := n.gatherSets(acc, nftables); err != nil {
return fmt.Errorf("gathering sets failed: %w", err)
}
case "anonymous-counters":
if err := n.gatherAnonymousCounters(acc, nftables); err != nil {
return fmt.Errorf("gathering anonymous counters failed: %w", err)
}
default:
return fmt.Errorf("unknown include %q", include)
}
}There was a problem hiding this comment.
I tried to fail as early as possible in Init(). Not sure if that matters.
The proposed implementation also implicitly deduplicates include = [ "counters", "counters" ] -- probably irrelevant.
Still prefer your suggestion?
There was a problem hiding this comment.
In Init you should check for
- valid values, i.e. error out on invalid/unknown strings
- duplicate values, i.e. construct a local
map[string]booland error out if you can already find that string
|
@srebhan Thanks for reviewing. I edited the summary to clarify the release status of the various features and asked some questions before my next revision. |
telegraf-tiger
bot
removed
the
waiting for response
|
Thanks @niklasf! Replied to your questions/comments... |
2 participants
Summary
#18134 recently added support for named sets, and #18177 fixes the parsing of named counters. This now fully supports named counters for consistency.
Additionally, supporting anonymous counters via comments on the rule feels a bit hacky when there is a dedicated way to name counters. Introduce a new option
anonymous_countersto allow turning this off, without changing the default behavior.cc @skartikey
Checklist
Related issues
based on #18135