-
Notifications
You must be signed in to change notification settings - Fork 748
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
cni-metrics-helper metrics: do type assertion before type casting #3152
Conversation
5394d02
to
b6be2b9
Compare
76e366a
to
2ae0160
Compare
} | ||
|
||
// Prometheus export supports only gauge metrics for now. | ||
|
||
func producePrometheusMetrics(t metricsTarget, families map[string]*dto.MetricFamily, convertDef map[string]metricsConvert) { | ||
func producePrometheusMetrics(t metricsTarget, families map[string]*dto.MetricFamily, convertDef map[string]metricsConvert) error { |
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.
@dshehbaj , this is not directly related to code, but still related.
In this function
// This can be enhanced to get it programatically.
// Initial CNI metrics helper enhancement includes only Gauge. Doesn't support GaugeVec, Counter, CounterVec and Summary
func GetSupportedPrometheusCNIMetricsMapping() map[string]prometheus.Collector {
var prometheusCNIMetrics = map[string]prometheus.Collector{
"awscni_eni_max": EnisMax,
"awscni_ip_max": IpMax,
"awscni_add_ip_req_count": AddIPCnt,
"awscni_del_ip_req_count": DelIPCnt,
"awscni_eni_allocated": Enis,
"awscni_total_ip_addresses": TotalIPs,
"awscni_assigned_ip_addresses": AssignedIPs,
"awscni_force_removed_enis": ForceRemovedENIs,
"awscni_force_removed_ips": ForceRemovedIPs,
"awscni_total_ipv4_prefixes": TotalPrefixes,
"awscni_no_available_ip_addresses": NoAvailableIPAddrs,
}
return prometheusCNIMetrics
}
The presence of
"awscni_add_ip_req_count": AddIPCnt,
"awscni_del_ip_req_count": DelIPCnt,
"awscni_force_removed_enis": ForceRemovedENIs,
"awscni_force_removed_ips": ForceRemovedIPs,
"awscni_no_available_ip_addresses": NoAvailableIPAddrs,
is wrong, and if we remove this, and then we don't need to typecast this too. Can we remove them from the above method? This exporter is only supposed to export Gauge metrics now.
2ae0160
to
de79aac
Compare
33df5e2
to
6efa143
Compare
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
What type of PR is this?
Bug fix
Which issue does this PR fix?:
#3144
Testing done on this change:
Added unit tests for code path for publishing CloudWatch and Prometheus Metrics and also pasted debug logs to confirm that no panic happens.
Built metrics helper image successfully using
make docker-metrics
.Unit tests passing
Debug logs before the change
Debug logs after the change