FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

Al2Klimov · 2023-11-08T11:29:38Z

This is basically the same as

Separately handle apply rules targetting only specific parent objects #9545

but for API filters, not for assign rules:

There's a filter on Hosts/Services
Does it unambiguously target only specific Hosts/Services?
If yes: fetch just them, don't run filter for all

This is unquestionably a significant performance improvement
If we're nice and backport it in 2.14.1, it will be -as a side effect- also conveniently available for testing regarding https://community.icinga.com/t/memory-leak-despite-2-14-upgrade-how-to-debug/12380/43

Al2Klimov · 2023-11-08T11:30:32Z

lib/remote/filterutility.cpp

+			if (dynamic_cast<ConfigObjectTargetProvider*>(provider.get())) {
+				auto dict (dynamic_cast<DictExpression*>(ufilter.get()));
+
+				if (dict) {
+					auto& subex (dict->GetExpressions());
+
+					if (subex.size() == 1u) {
+						if (type == "Host") {


There's a filter on Hosts/Services

Al2Klimov · 2023-11-08T11:30:52Z

lib/remote/filterutility.cpp

+						if (type == "Host") {
+							std::vector<const String *> targetNames;
+
+							if (GetTargetHosts(subex.at(0).get(), filter_vars, targetNames)) {


Does it unambiguously target only specific Hosts/Services?

Al2Klimov · 2023-11-08T11:31:06Z

lib/remote/filterutility.cpp

+			if (targeted) {
+				for (auto& target : targets) {
+					if (FilterUtility::EvaluateFilter(permissionFrame, permissionFilter.get(), target, variableName)) {
+						result.emplace_back(std::move(target));
+					}
+				}


If yes: fetch just them, don't run filter for all

Al2Klimov · 2023-11-09T14:35:44Z

Test

$ time curl -ksSu root:123456 -o /dev/null -H 'Accept: application/json' -X GET -d '{"filter":"y == service.name && x == host.name","filter_vars":{"x":"alexandersmbp2.int.netways.de","y":"42"}}' 'https://127.0.0.1:5665/v1/objects/services'
curl -ksSu root:123456 -o /dev/null -H 'Accept: application/json' -X GET -d    0,01s user 0,01s system 22% cpu 0,058 total
$ time curl -ksSu root:123456 -o /dev/null -H 'Accept: application/json' -X GET -d '{"filter":"y == service.name && x == host.name && 1","filter_vars":{"x":"alexandersmbp2.int.netways.de","y":"42"}}' 'https://127.0.0.1:5665/v1/objects/services'
curl -ksSu root:123456 -o /dev/null -H 'Accept: application/json' -X GET -d    0,01s user 0,01s system 0% cpu 3,290 total
$

👍 The above one is 50x faster, just because no && 1

Config

for (i in range(100000)) {
        object Service i {
                host_name = NodeName
                check_command = "passive"
                enable_active_checks = false
        }
}

yhabteab

Apart from the comments below it's just fine, I'm not sure how this would in any way fix the memory leak referenced in the PR desc though.

lib/config/applyrule.hpp

yhabteab · 2023-12-13T13:42:56Z

lib/remote/filterutility.cpp

+						if (type == "Host") {
+							std::vector<const String *> targetNames;
+
+							if (ApplyRule::GetTargetHosts(subex.at(0).get(), targetNames, filter_vars)) {


Since the goal of this PR is reducing the performance overhead, you should favour subex[0] over subex.at(0).

Won't give much in addition, at the cost of SEGV -not just exception- if the code breaks in the future.

yhabteab · 2023-12-13T14:23:01Z

lib/remote/filterutility.cpp

+						} else if (type == "Service") {
+							std::vector<std::pair<const String *, const String *>> targetNames;
+
+							if (ApplyRule::GetTargetServices(subex.at(0).get(), targetNames, filter_vars)) {


yhabteab · 2023-12-13T14:36:15Z

test/config-apply.cpp

I think it's better to use BOOST_(UN)EQUAL(), BOOST_REQUIRE_(UN)EQUAL() instead of BOOST_CHECK() as Julian suggested in another PR of yours.

Doesn't work for vectors unfortunately.

in addition to literal strings. This is for sandboxed filters with some variables pre-set by the caller. They're "constant" in that scope, too.

…ct(s) for all objects

test/config-apply.cpp

yhabteab · 2023-12-18T13:24:39Z

test/config-apply.cpp

+			actualServiceNames.emplace_back(*s.first, *s.second);
+		}
+
+		BOOST_CHECK(actualServiceNames == services);


It should also be possible to replace this with BOOST_CHECK_EQUAL_COLLECTIONS() by implementing the output operator for the std::pair<String, String> type.

template<> struct boost::test_tools::tt_detail::print_log_value<std::pair<String, String>> { inline void operator()(std::ostream& os, const std::pair<String, String>& hs) { os << hs.first << "!" << hs.second; } };

Not bad! Please open a PR into my PR.

Or push directly here. At your option.

…TIONS() to show the value diff in case of mismatch. Co-authored-by: Yonas Habteab <[email protected]>

yhabteab

As an optimisation of the object lookup for simple API filters targeting the Service or/and Host names LGTM! However, this is unlikely to fix the memory leak described in the community form, as we don't even know where it is leaking.

Al2Klimov added enhancement New feature or request area/api REST API consider backporting Should be considered for inclusion in a bugfix release labels Nov 8, 2023

Al2Klimov added this to the 2.15.0 milestone Nov 8, 2023

Al2Klimov requested a review from julianbrost November 8, 2023 11:29

cla-bot bot added the cla/signed label Nov 8, 2023

icinga-probot bot added area/configuration DSL, parser, compiler, error handling core/quality Improve code, libraries, algorithms, inline docs ref/IP labels Nov 8, 2023

Al2Klimov commented Nov 8, 2023

View reviewed changes

Al2Klimov force-pushed the targeted-api-filter branch from af3f5a4 to f52fbc0 Compare November 22, 2023 10:04

Al2Klimov requested a review from yhabteab December 12, 2023 11:24

yhabteab requested changes Dec 13, 2023

View reviewed changes

Al2Klimov added 4 commits December 13, 2023 16:02

Introduce Dictionary#GetRef()

8bcae97

Introduce DictExpression#GetExpressions()

a04cef1

ApplyRule::GetTarget*s(): support constant strings from variables

15191bc

in addition to literal strings. This is for sandboxed filters with some variables pre-set by the caller. They're "constant" in that scope, too.

FilterUtility::GetFilterTargets(): don't run filter for specific obje…

ecfc903

…ct(s) for all objects

Al2Klimov force-pushed the targeted-api-filter branch from f52fbc0 to 687309e Compare December 13, 2023 15:28

Al2Klimov requested a review from yhabteab December 13, 2023 15:28

yhabteab requested changes Dec 18, 2023

View reviewed changes

test/config-apply.cpp Outdated Show resolved Hide resolved

test/config-apply.cpp Outdated Show resolved Hide resolved

test/config-apply.cpp Outdated Show resolved Hide resolved

test/config-apply.cpp Outdated Show resolved Hide resolved

Test ApplyRule::GetTarget*s()

191bf93

Al2Klimov force-pushed the targeted-api-filter branch from 687309e to 191bf93 Compare December 18, 2023 12:07

Al2Klimov requested a review from yhabteab December 18, 2023 12:08

yhabteab reviewed Dec 18, 2023

View reviewed changes

Tests: config_apply/gettargetservices_*: use BOOST_CHECK_EQUAL_COLLEC…

4424d57

…TIONS() to show the value diff in case of mismatch. Co-authored-by: Yonas Habteab <[email protected]>

Al2Klimov requested a review from yhabteab December 18, 2023 15:07

yhabteab approved these changes Dec 18, 2023

View reviewed changes

Al2Klimov merged commit 949d983 into master Dec 19, 2023
25 checks passed

Al2Klimov deleted the targeted-api-filter branch December 19, 2023 14:18

Al2Klimov mentioned this pull request Dec 19, 2023

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9944

Merged

Al2Klimov added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels May 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

Al2Klimov commented Nov 8, 2023

Al2Klimov Nov 8, 2023

Al2Klimov Nov 8, 2023

Al2Klimov Nov 8, 2023

Al2Klimov commented Nov 9, 2023

yhabteab left a comment

yhabteab Dec 13, 2023

Al2Klimov Dec 13, 2023

yhabteab Dec 13, 2023

yhabteab Dec 13, 2023

Al2Klimov Dec 13, 2023

yhabteab Dec 18, 2023

Al2Klimov Dec 18, 2023

Al2Klimov Dec 18, 2023

yhabteab left a comment

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

FilterUtility::GetFilterTargets(): don't run filter for specific object(s) for all objects #9895

Conversation

Al2Klimov commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Al2Klimov commented Nov 9, 2023

Test

Config

yhabteab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yhabteab left a comment

Choose a reason for hiding this comment