NETOBSERV-1447: Added a check to log a warning if some labels are used in metric defi… #629
Conversation
|
/retest |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #629 +/- ##
==========================================
- Coverage 67.01% 67.00% -0.02%
==========================================
Files 68 68
Lines 7783 7804 +21
==========================================
+ Hits 5216 5229 +13
- Misses 2192 2197 +5
- Partials 375 378 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/lgtm |
|
building cluster with this change for testing. |
|
I was able to see warning in Operator logs: I have some feedback for this warning to be more visible, we can have this PR merged. /label qe-approved |
jotak
changed the title
|
@OlivierCazade: This pull request references NETOBSERV-1447 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
pkg/helper/helpers.go
Outdated
|
|
||
| func LabelIsHighCardinality(label string) bool { | ||
| pluginCfg := config.PluginConfig{} | ||
| err := yaml.Unmarshal(config.LoadStaticFrontendConfig(), &pluginCfg.Frontend) |
There was a problem hiding this comment.
It would be good to cache the PluginConfig rather than unmarshaling it every time (not just here but also done in other places). Especially here that's done in a for loop for each label
There was a problem hiding this comment.
we could leverage https://github.com/netobserv/network-observability-operator/blob/main/pkg/helper/helpers.go#L95 remove the for loop and pass in another bool isHighCardinality or do something similar and this will do unmarshall once WDYT?
something like
func FindFilter(labels []string, isNumber, isHighCardinality bool) bool {
var cfg config.FrontendConfig
type filter struct {
exists bool
isNum bool
isHighCardinality bool
}
err := yaml.Unmarshal(config.LoadStaticFrontendConfig(), &cfg)
if err != nil {
return false
}
labelMap := make(map[string]filter)
for _, f := range cfg.Fields {
value := filter{
exists: true,
isNum: f.Type == "number",
isHighCardinality: f.CardinalityWarn == config.CardinalityWarnCareful || f.CardinalityWarn == config.CardinalityWarnAvoid,
}
labelMap[f.Name] = value
}
for _, l := range labels {
if ok := labelMap[l].exists; !ok {
return false
}
if isNumber && !labelMap[l].isNum {
return false
}
if isHighCardinality && labelMap[l].isHighCardinality {
return true
}
}
return true
}There was a problem hiding this comment.
I modified LoadStaticFrontendConfig to unmarshall inside the function and only once.
| cfgLabel.CardinalityWarn == config.CardinalityWarnAvoid | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
this works for me thank you!!
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Added a check to log a warning if some labels are used in metric definition
Dependencies
n/a
Checklist
If you are not familiar with our processes or don't know what to answer in the list below, let us know in a comment: the maintainers will take care of that.