-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat(metrics-operator): introduce scoring logic for Analysis evaluations #1872
Conversation
✅ Deploy Preview for keptn-lifecycle-toolkit ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1872 +/- ##
==========================================
+ Coverage 83.53% 83.87% +0.33%
==========================================
Files 107 141 +34
Lines 7077 8588 +1511
==========================================
+ Hits 5912 7203 +1291
- Misses 957 1128 +171
- Partials 208 257 +49
... and 32 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
30b16c8
to
8c464c3
Compare
metrics-operator/controllers/common/analysis/analysis_evaluator_test.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/taget_evaluator_test.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/taget_evaluator_test.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/types/types_test.go
Outdated
Show resolved
Hide resolved
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
metrics-operator/controllers/common/analysis/analysis_evaluator.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/analysis_evaluator.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/analysis_evaluator.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/objective_evaluator.go
Outdated
Show resolved
Hide resolved
metrics-operator/controllers/common/analysis/objective_evaluator.go
Outdated
Show resolved
Hide resolved
f9dd457
to
31a8ff4
Compare
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
Signed-off-by: odubajDT <ondrej.dubaj@dynatrace.com>
9ba1f42
to
e66ee7c
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
// get the value | ||
floatVal, err := getValueFromMap(values, computeKey(obj.AnalysisValueTemplateRef)) | ||
if err != nil { | ||
result.Error = err |
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 know if this error message makes sense to be stored in the CR 🤔
looking at the code, it seems that this .Error
field is not used. Maybe we could propagate the error up so we can log it at the entry point with info about what analysis fails
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 error is filled when the map of values does not contain the required key and value, therefore we store the error that the value for the objective was not retrieved and pass the result one layer above. Therefore in the end, each ObjectiveResult
has an error stored in the structure if it occured
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 struct is only internally used and it is not stored in any CR. However, the field it is not used so I am questioning the utility of storing errors. Furthermore, we are going against Go practices to have the error flow as part of the function return type and instead, we hide that into the returning struct :/
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.
But we should store the error, when the value cannot be retrieved. It needs to be stored within the objectiveResult. You are right that we do not need to store the whole error, but just the error message.
If we just return the error as a function return type, we do not have any information in the resulting status why this particular objective has score == 0. We might lose information here then
Changes
Fixes: #1767