-
Notifications
You must be signed in to change notification settings - Fork 95
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
THREESCALE-7961: Use sum_irate instead of sum_rate #740
Conversation
Add logic to use `sum_irate` in monitoring queries when the Openshift version is greater or equal than 4.9. Keep using `sum_rate` when the Openshift version is less than 4.9.
pkg/helper/openshift.go
Outdated
// AddSumRateField creates a templateDataMutation that constructs a struct | ||
// identical to the original with the added `SumRate` field, containing the | ||
// value of sumRate | ||
func AddSumRateField(sumRate string) TemplateDataMutation { |
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 understand the motivation of this implementation based on the reflect
package.
I am ok with it, but I would like to share that IMHO the complexity added to solve a relatively simple use case does not pay off.
Some (if not all) the dashboard templates have a new parameter called SumRate
. I would add them to the data struct and the value passed to the SumRate
will be computed based on the logic you implemented.
For example, in pkg/3scale/amp/component/generic_monitoring.go
func KubernetesResourcesByNamespaceGrafanaDashboard(sumRate string, ns string, appLabel string) *grafanav1alpha1.GrafanaDashboard {
data := &struct {
Namespace string
SumRate string
}{
ns, sumRate,
}
return &grafanav1alpha1.GrafanaDashboard{
ObjectMeta: metav1.ObjectMeta{
Name: "kubernetes-resources-by-namespace",
Labels: map[string]string{
"monitoring-key": common.MonitoringKey,
"app": appLabel,
},
},
Spec: grafanav1alpha1.GrafanaDashboardSpec{
Json: assets.TemplateAsset("monitoring/kubernetes-resources-by-namespace-grafana-dashboard-1.json.tpl", data),
Name: fmt.Sprintf("%s/kubernetes-resources-by-namespace-grafana-dashboard-1.json", ns),
},
}
}
The caller would be:
sumRate, err := helper.SumRateForOpenshiftVersion(r.Context(), r.Client())
if err != nil {
return reconcile.Result{}, err
}
grafanaDashboard := component.KubernetesResourcesByNamespaceGrafanaDashboard(sumRate, r.apiManager.Namespace, *r.apiManager.Spec.AppLabel)
err = r.ReconcileGrafanaDashboard(grafanaDashboard, reconcilers.GenericGrafanaDashboardsMutator)
if err != nil {
return reconcile.Result{}, 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.
Thanks @eguzki I followed your suggestion and simplified the inclusion of the SumRate
parameter into the templates
9e3e092
to
595a0df
Compare
595a0df
to
03a3178
Compare
pkg/helper/openshift.go
Outdated
return sumRate, nil | ||
} | ||
|
||
type TemplateDataMutation func(interface{}) interface{} |
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 being used?
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.
Good catch, I missed it when removing the stuff from the initial implementation. Fixed it now
Code Climate has analyzed commit a626076 and detected 31 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Description
Add logic to use
sum_irate
in monitoring queries when the Openshift version is greater or equal than 4.9. Keep usingsum_rate
when the Openshift version is less than 4.9.Verification steps
sum_rate
use nowsum_irate
and show datasum_rate
and show data