icinga, cluster: add option to limit perfdata #8403
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
icinga-probot
bot
added
area/checks
Al2Klimov
force-pushed
the
feature/limit-perfdata-8124
branch
6 times, most recently
from
5a176a7 to
c46a30c
Compare
|
Al2Klimov
force-pushed
the
feature/limit-perfdata-8124
branch
from
c46a30c to
2ce1287
Compare
|
@cla-bot check |
julianbrost
reviewed
Mar 13, 2023
There was a problem hiding this comment.
#8124 mentions a few more checks than what is done is this PR:
"icinga", "cluster", "custer-zone" and "ido"
Did you omit cluster-zone because it only emits 7 perfdata values? Seems low enough that I think it would be absolutely necessary to filter here. @dgoetz Would there be a particular need for filters on that check?
ido could be fine to omit as it's effectively deprecated, but the icingadb check also emits lots of perfdata, so there might very well be the desire to filter here as well.
In general, would it make to sense to provide the ability to filter perfdata values as an option for all check commands? That would avoid that we have to hand pick the internal checks we want to implement this for and that would sound equally useful for external checks as well. Quick cross-reference that just came to my mind: implementing something like the suggestion in #6937 (comment) should allow this and would solve other issues as well.
Comment on lines
+1915
to
+1925
| if (!filter.IsObject()) { | ||
| return; | ||
| } | ||
|
|
||
| auto filterArray (dynamic_pointer_cast<Array>((Object::Ptr)filter)); | ||
|
|
||
| if (!filterArray) { | ||
| return; | ||
| } | ||
|
|
||
| auto filterStrings (filterArray->ToSet<String>()); |
There was a problem hiding this comment.
Invalid configuration shouldn't be ignored silently like the first two ifs do (except in the filter == Empty case, but that's valid config). Without having tested this, this also looks to be inconsistent: with (in DSL syntax) filter = 42 it would be ignored whereas I thing filter = [[42]] would throw an exception as [42] can't be casted to String.
Comment on lines
+1927
to
+1952
| for (decltype(perfdata->GetLength()) i = 0; i < perfdata->GetLength(); ++i) { | ||
| auto item (perfdata->Get(i)); | ||
|
|
||
| if (!item.IsObject()) { | ||
| continue; | ||
| } | ||
|
|
||
| auto itemPerfdataValue (dynamic_pointer_cast<PerfdataValue>((Object::Ptr)item)); | ||
|
|
||
| if (!itemPerfdataValue) { | ||
| continue; | ||
| } | ||
|
|
||
| auto label (itemPerfdataValue->GetLabel()); | ||
|
|
||
| for (auto& pattern : filterStrings) { | ||
| if (Utility::Match(pattern, label)) { | ||
| // continue 2; | ||
| goto NextItem; | ||
| } | ||
| } | ||
|
|
||
| perfdata->Remove(i--); | ||
|
|
||
| NextItem:; | ||
| } |
There was a problem hiding this comment.
This should be a good place to use std::remove_if. That would avoid the need for goto here and also has linear complexity (though questionably if anyone would notice a difference here).
|
@lippserd What do you think? Custom vars (as-is, |
|
FYI I personally prefer to close this and -depending on customers' needs- either do nothing at all or let the users do this by themselves via this feature as Julian described: #6937 (comment) |
The feature you refer to is not implemented iirc. |
|
This one here is neither merged. I prefer one feature you can use twice rather than two features. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #8124