Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #20310: Method getByRulesCompliance used by API is highly inefficient #4017

Merged
merged 5 commits into from Dec 6, 2021
Merged

Fixes #20310: Method getByRulesCompliance used by API is highly inefficient #4017

merged 5 commits into from Dec 6, 2021

Conversation

ncharles
Copy link
Member

@ncharles ncharles requested a review from fanf November 24, 2021 22:09
@ncharles ncharles added Trigger test WIP Use that label for a Work In Progress PR that must not be merged yet labels Nov 24, 2021
@ncharles
Copy link
Member Author

This PR is not finished, but so far:

  • API calls to compliance/rules go from 17s to 14s (it returns 229MB of data)
  • it changes what is returned however: Rules not applied on anything don't show up in the resulting list - i'm unsure whether it's good or not, at least for me it seems to be a change that make sense (no compliance is not 0 compliance)

@ncharles
Copy link
Member Author

PR updated with a new commit

@ncharles
Copy link
Member Author

This PR changes what is returned: Rules not applied on anything don't show up in the resulting list - i'm unsure whether it's good or not, at least for me it seems to be a change that make sense (no compliance is not 0 compliance)

API calls to compliance/rules go from 17s to 14s with 4950 nodes (include js serialization)
Without writing json output:
API calls for level1 goes from 5s to 1s
API calls for level2 goes from 5s to 0.6s
API calls for level3 stays at 5.6s

@ncharles
Copy link
Member Author

PR updated with a new commit

@ncharles ncharles removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Nov 26, 2021
@ncharles
Copy link
Member Author

actually, we should not change the list of rules returns, as it is used in 7.0 for rule pages

@ncharles ncharles added WIP Use that label for a Work In Progress PR that must not be merged yet Trigger test and removed Trigger test labels Nov 30, 2021
@ncharles
Copy link
Member Author

PR updated with a new commit

VinceMacBuche
VinceMacBuche reviewed Nov 30, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
@@ -98,7 +97,12 @@ class ComplianceApi(

(for {
level <- restExtractor.extractComplianceLevel(req.params)
rules <- complianceService.getRulesCompliance()
computeLevel <- Full(if(version.value <= 6) {
None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Some(10) (or Some(value expected for api versio 6 )

But Version 6 is not supported anymore with we just may want to remove them (need to check though), but i think 6.1 is 7 minimal api version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few line below you can use compluteLevel insted of level.getOrElse(10)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this fails if level is not defined, if defined and version > 6 we should default to default vlaue (10)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I misread i haven't seen the Full, nevermind this is ok !

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

level is an option that appears only later, so I'd like to keep the semantic of "not defined" , thus an None

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes i just misread and haven't seen it was in a Full and thought the None could break the computation, but it's not the case at all

@ncharles
Copy link
Member Author

ncharles commented Dec 1, 2021

PR updated with a new commit

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
for {
groupLib <- nodeGroupRepo.getFullGroupLibrary().toBox
directivelib <- directiveRepo.getFullDirectiveLibrary().toBox
t2 = System.currentTimeMillis()
_ = logger.trace(s"getByRulesCompliance - getFullGroupLibrary in ${t2 - t1} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~400ms

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timings for 4950 nodes, with API answering in 1.2s

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
nodeInfos <- nodeInfoService.getAll()
t4 = System.currentTimeMillis()
_ = logger.trace(s"getByRulesCompliance - nodeInfoService.getAll() in ${t4 - t3} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ 8ms

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
)
} yield {
t6 = System.currentTimeMillis()
_ = logger.trace(s"getByRulesCompliance - findRuleNodeStatusReports in ${t6 - t5} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ 170ms

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
}
val reportsByRule = reportsByNode.flatMap { case(_, status) => status.reports }.groupBy( _.ruleId)
val t7 = System.currentTimeMillis()
logger.trace(s"getByRulesCompliance - group reports by rules in ${t7 - t6} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~150 ms

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
(initializedCompliances ++ nonEmptyRules).values.toSeq
}
val t8 = System.currentTimeMillis()
logger.trace(s"getByRulesCompliance - Compute non empty rules in ${t8 - t7} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~120ms

ncharles
ncharles commented Dec 1, 2021
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
}

val t9 = System.currentTimeMillis()
logger.trace(s"getByRulesCompliance - Compute ${initializedCompliances.size} empty rules in ${t9 - t8} ms")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ 60ms

@ncharles ncharles removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Dec 3, 2021
@ncharles ncharles mentioned this pull request Dec 3, 2021
Merged
@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/4017
-- Your faithful QA
Kant merge: "To be is to do."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/47895/console)

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

9 similar comments
@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@amousset
Copy link
Member

amousset commented Dec 6, 2021

OK, squash merging this PR

@amousset
Copy link
Member

amousset commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, squash merging this PR

@ncharles
# This is a combination of 2 commits.
66ad4a8 
# This is the 1st commit message:

Fixes #20310: Method getByRulesCompliance used by API is highly inefficient

# The commit message #2 will be skipped:

# fixup! Fixes #20310: Method getByRulesCompliance used by API is highly inefficient
#
# Fixes #20310: Method getByRulesCompliance used by API is highly inefficient
@ncharles
Fixes #20310: Method getByRulesCompliance used by API is highly ineff…
39dcc90 
…icient
@ncharles
Fixes #20310: Method getByRulesCompliance used by API is highly ineff…
74eaed5 
…icient
@ncharles
Fixes #20310: Method getByRulesCompliance used by API is highly ineff…
e8d848f 
…icient
@ncharles
Fixes #20310: Method getByRulesCompliance used by API is highly ineff…
d6cb387 
…icient
@ncharles ncharles force-pushed the bug_20310/method_getbyrulescompliance_used_by_api_is_highly_inefficient branch from cc7c9a2 to d6cb387 Compare December 6, 2021 20:29
@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, merging this PR

@ncharles ncharles merged commit d6cb387 into Normation:branches/rudder/6.1 Dec 6, 2021
@ncharles
Copy link
Member Author

ncharles commented Dec 6, 2021

OK, merging this PR

@VinceMacBuche VinceMacBuche mentioned this pull request Dec 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VinceMacBuche VinceMacBuche VinceMacBuche approved these changes

@fanf fanf Awaiting requested review from fanf

Assignees
No one assigned
Labels
qa: Can't merge Trigger test
Projects
None yet
Milestone
No milestone
4 participants
@ncharles @Normation-Quality-Assistant @amousset @VinceMacBuche