Fixes #20607: Make possible to choose precision in compliance percent API #4127
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
ncharles
reviewed
Jan 19, 2022
| * (given that we have 14 categories, it means that at worst, if 13 are at 0.1 in place of ~0%; | ||
| * the last one will be false by 0.13%, which is ok) | ||
| * - always keeps at least 10e(-precision)% for any case | ||
| * (given that we have 14 categories, it means that at worst, if 13 are at the mean in place of ~0%; |
There was a problem hiding this comment.
Suggested change
| * (given that we have 14 categories, it means that at worst, if 13 are at the mean in place of ~0%; | |
| * (given that we have 14 categories, it means that at worst, if 13 are at the min in place of ~0%; |
amousset
reviewed
Jan 19, 2022
| type: integer | ||
| example: 0 | ||
| default: 2 | ||
| description: Number of digits after comma in compliance percent figures |
There was a problem hiding this comment.
we can maybe precise here that a low value will never be zero and that is it not a simple rounding.
ncharles
approved these changes
Jan 19, 2022
|
OK, merging this PR |
|
This is a nice change. I'm indeed not sure of the impact of the lazy val - i did not notice anything significant in my tests - i'm still unsure if we can do better than that or not, but that will be for a next iteration, as perfs are satisfactory |
fanf
deleted the
bug_20607/make_possible_to_choose_precision_in_compliance_percent_api
branch
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.
https://issues.rudder.io/issues/20607
Superseed #4115 and keep #4110 for correction of https://issues.rudder.io/issues/20573
The idea is to be able to specify what precision we want for compliance percent in the API calls, and let the backend compute compliance with that value. Most of the pr is passing the
precisionparameter around.Note that we can't specify
precisionvalue for thelazy val pcattribute inComplianceLevelinstance, so a default value of2is kept. Thatpcvalue does not seems to be used in the compliance displaying for UI, so it makes me wonder if we really need an heavy lazy val in eachComplianceLevelobjects, perhaps we should only compute it when needed withCompliancePercent.fromLevels(c, precision)- cc @ncharles for that.The 2nd commit totaly remove
lazy valfromComplianceLevelobject. It will free 3 object alloc by compliance level object, which is not nothing given the number of short lived ones we have, and it seems that there is not many places where they are reused (and where it can't be computed before the different use place).