-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
Fixes #23967: Rudder score backend and API #5296
Fixes #23967: Rudder score backend and API #5296
Conversation
Commit modified |
88904b5
to
a74abb4
Compare
Commit modified |
PR updated with a new commit |
385e026
to
db81b09
Compare
Commit modified |
db81b09
to
517e3c5
Compare
Commit modified |
517e3c5
to
cff2251
Compare
Commit modified |
cff2251
to
5b03248
Compare
Commit modified |
5b03248
to
70d93b4
Compare
Create table GlobalScore ( | ||
nodeId text primary key | ||
, score text | ||
, message text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is that message for? I would prefer to use name
(for a short human name), description
(for a one line description for ex on mouse over) or documentation
(for a longer, possibly semi-structured/mkd explanation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the score explanation message. I think it is really a message. It is not a configuraiton structure and i do think it should not follow the same rules than other items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Create table scoreDetails ( | ||
nodeId text | ||
, score text | ||
, name text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
should be scoreId
I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want
object ComplianceSerializable { | ||
def fromPercent(compliancePercent: CompliancePercent) = { | ||
ComplianceSerializable( | ||
if (compliancePercent.pending == 0) None else Some(compliancePercent.pending), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should have a private def to avoid duplicating that logic and avoid risking micopying the name in both place
unexpectedMissingComponent: Option[Double], | ||
noReport: Option[Double], | ||
reportsDisabled: Option[Double], | ||
badPolicyMode: Option[Double] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the elements are not in the same order than CompliancePercent
, that seems to be an extremely easy way to have errors.
For the naming, it seems better to use the event name like you did, but I wonder why we don't use them in CompliancePercent too.
) extends PipelinedInventorySaver[Unit] { | ||
|
||
override def commitChange(inventory: Inventory): IOResult[Unit] = { | ||
implicit val cc = ChangeContext.newForRudder() | ||
backend.updateInventory(FullInventory(inventory.node, Some(inventory.machine)), Some(inventory.applications)).unit | ||
backend.updateInventory(FullInventory(inventory.node, Some(inventory.machine)), Some(inventory.applications)).unit *> | ||
scoreServiceManager.handleEvent(InventoryScoreEvent(inventory.node.main.id, inventory)).unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why puting it here and not in a PostCommit
handler? They should be able to match that use case, and I don't see the need for hard implementation coupling here. And I don't think it gives any additional consistency benefits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
caa95a6
to
b7f81cc
Compare
Commit modified |
|
||
Create table scoreDetails ( | ||
nodeId text | ||
, score text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be an enumerated type, it seems to be the perfect case for that, and it would likely help regarding perf and size. But I don't know what is the migration story if for ex we want to add new values.
, message text | ||
, details jsonb | ||
, PRIMARY KEY (nodeId, name) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can add not null not empty constraints for nodeId, score, name
} | ||
} | ||
|
||
trait Score[T] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to have that parametrization. We never use a generic access to details, and List[Score[_]]
is a signal that we maybe are not doing things like we should.
Plus, that parametrization risk to create some unpleasant situation since Score[ComplianceSerializable]
is not the same as ComplianceScore
(at type level), but in our code, we really really want to have that equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like the first commit in that PR: VinceMacBuche#29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact we could remove all ComplianceScore, and juste avec a Score that has a Json as details. Just everything translated into a jsonScore directly from the score event
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to have different score implementation for the different cases, it makes the score computation very clean and really feels like each module can do whatever they want from a structured data type, and not Json. But perhaps I misunderstood ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes ScoreEvent will be different witth diffrent structure.
} | ||
|
||
trait ScoreSerializerService { | ||
def parse(score: Score[Json]): IOResult[Option[Score[_]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a pure result, I don't see why we should have IO here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i need some services that may not be pure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see any and parsing should really always be pure (and perhaps you need some impure one before geting the data, but having IO involved in the parsing feels really like not what we want to have to deal with)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's uised mostly in ZIO context and i have to transform my PureResult into IORestult to i guess it does not gain much
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it gains in intent conveyed, and in unit test easyness. IOResult
is a bit like global variables, they are easy and powerfull, but we should always try to restraint as much as possible to use them because they are too powerful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not a subject anymore since that part of the code disapeared, it's for info)
|
||
trait ScoreSerializerService { | ||
def parse(score: Score[Json]): IOResult[Option[Score[_]]] | ||
def toJson(score: Score[_]): IOResult[Option[Score[Json]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how it can fail. I understand to potential None
case if there is no serializer for that score, but if there is one, it should not ever fail (and so just be a Option[Score[Json]]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if that can't be simplier.
It's used in two place:
- in DB to read/write from a JSON column (so a string), and using the intermediary
Json
means that we dodb -> get String -> parse to Json -> serialize to String -> parse to T
- we could certainly directly use the string, even if that means having aStringScore
to simplify doobie interaction, - in node API, to serialize these info. But here, we should really just need the
JsonEncoder
which we already have (and again avoid theT -> to String -> parse Json -> to String
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General idea draft in the second commit of that PR: VinceMacBuche#29 (not sure it floats till the end, but we can in the worst case keep JsonScore
for NodeAPI
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to remove invalid score (that are not json) at the beginning. and it will end up parsed as json (either in api, could be also used to init score if we wanted to recompute a score ) I don't think there is a usage in using a String and not a Json, which is the underlying type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have everything to manage json from database. and here you have to translate into json at the end which can be already done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still check for invalid data structure at the begining, it doesn't change that. You certainly can use directly the JsonEncoder
and JsonDecoder
in the score repository, I just didn't take the time to implem it.
Here, each time you use as
or toJsonAST
, you really go through a string parsing in the middle.
And I really believe we should not have one unique JsonScore, it will make the score calculation part less clear.
Commit modified |
504926a
to
452574c
Compare
Commit modified |
452574c
to
a629025
Compare
Commit modified |
a629025
to
8cdb57e
Compare
|
||
object ComplianceScoreEventHandler extends ScoreEventHandler { | ||
implicit val compliancePercentEncoder: JsonEncoder[ComplianceSerializable] = DeriveJsonEncoder.gen | ||
def handle(event: ScoreEvent): PureResult[List[(NodeId, List[Score])]] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we always have that structure:
- a
Score
computation from a subtype ofScoreEvent
- a partial function from a
ScoreEvent
to aScore
(ie a "can handle that type" + "if yes, use the computation function with the correct subtype of score event"
I think you need to split appart these two bits so that the score from score event computation
is unit-testable by itself on two aspects:
- the correctness of the computation (easy here, but it will be a kind of spec so users will question it a lot, we need to be able to accumulate all the strange corner case they will find)
- the JSON format, because it becomes a storage format that will need to never change without a migration
Plus, once you have that separation, the implementation of the handle
will be almost automatic, so more easy for other people needing to do so to understand what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for me, once spotless/tests pass.
We tracked all the non blocking change elsewhere for latter PR.
main = | ||
Browser.element | ||
{ init = init | ||
, view = always (Html.text "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intended to not display anything ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! these apps only exist to provide details for generic node details api that does not know how to handle them (in fact it could handle system-updates and compliance by itself but i wanted them to be like the plugins so that we know the recipe to make a plugin from Rudder
@@ -393,6 +397,7 @@ trait CachedFindRuleNodeStatusReports | |||
// * no report from the node (compliance expires): recompute compliance | |||
{ | |||
(for { | |||
_ <- ReportLoggerPure.warn(s"Hello ! ${actions} ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not forget to remove this !
implicit val scoreWithIdRead: Read[(NodeId, Score)] = { | ||
Read[(String, ScoreValue, String, String, Json)].map { d: (String, ScoreValue, String, String, Json) => | ||
(NodeId(d._1), Score(d._3, d._2, d._4, d._5)) | ||
} | ||
} | ||
|
||
import doobie._ | ||
override def getAll(): IOResult[Map[NodeId, List[Score]]] = { | ||
val q = sql"select nodeId, score, name, message, details from scoreDetails " | ||
transactIOResult(s"error when getting scores for node")(xa => q.query[(NodeId, Score)].to[List].transact(xa)) | ||
.map(_.groupMap(_._1)(_._2)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those tuple indices look really confusing between both implicits (but they look correct)
Normally doobie can generate the Read[(NodeId, Score)]
by re-using the Read[Score]
so we can remove the noise from scoreWithIdRead
here
Commit modified |
75e02da
to
9445f91
Compare
Commit modified |
9445f91
to
ea1469d
Compare
PR updated with a new commit |
} | ||
|
||
transactIOResult(s"error when inserting global score for node '${nodeId.value}''")(xa => | ||
query.updateWithLabel("test").run.transact(xa) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test !
, score text | ||
, message text | ||
, details jsonb | ||
, PRIMARY KEY (nodeId, scoreIdé) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scoreIdé !
PR updated with a new commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it only misses the database migration and the not null constraint ! (And of course to fix the compilation errors)
Commit modified |
4e54e58
to
c053316
Compare
Commit modified |
c053316
to
f91eacc
Compare
PR rebased |
f91eacc
to
df8b61d
Compare
OK, merging this PR |
df8b61d
into
Normation:branches/rudder/8.1
https://issues.rudder.io/issues/23967