Metrics (3/4) #550
Metrics (3/4) #550
Conversation
related to #124 In this pull request I implemented the metrics system in k8gb. I added one metric - counter incrementing the number of successful reconciliation loops. Adding specific metrics is part of the next PR. Like the logger, the metrics are accessed through a single singleton object and like the logger it is initialized. ```go var log = logging.Logger() var m = metrics.Metrics() ``` then anywhere in the code we can use this: ``` // increase the number of successful reconciliations m.ReconciliationIncrement() ``` The singleton is initialized in the `main()` function using the `metrics.Init(*config)` function. In case the singleton is not initialized, it will be set to the default value and all metrics will start with the prefix `k8gb_default` otherwise they will start with the prefix from the env variable `K8GB_NAMESPACE`. In addition to the singleton, I have changed the original metrics and added tests at the package level. In addition to the singleton, I significantly refactored the original metrics and added tests at the package level. The prometheus metrics implementation uses reflection. In short, I read the collectors structure during the runtime: ```go type collectors struct { HealthyRecords *prometheus.GaugeVec IngressHostsPerStatus *prometheus.GaugeVec ZoneUpdateTotal prometheus.Counter ReconciliationTotal prometheus.Counter } ``` Now I know all the types and names. From this I generate the map <name>: <instance>. I will then use the map for Register(), Unregister() and Get(). <name> is the name of the metric and is generated from the name of the structure in `collectors`. ReconciliationTotal => for example: `k8gb_gslb_healthy_records` (k8gb is `K8GB_NAMESPACE`) `Get()` is only for testing purposes - and is public only because controller_tests use metrics. I'll consider deleting tests from controller_tests and leaving only terratests and unit_tests at the package level (96% coverage now). But that will come in the next PR. Signed-off-by: kuritka <kuritka@gmail.com>
kuritka
requested review from
donovanmuller,
k0da,
somaritane and
ytsarev
as code owners
|
|
||
| var ( | ||
| o sync.Once | ||
| metrics = nop() |
There was a problem hiding this comment.
Could you explain this nop() thing?
There was a problem hiding this comment.
Good question, thx:
first of all, the logger / metrics recorder should have a default state even if the metrics.Init(config) function is not called yet. This doesn't matter during runtime, because Init(config) is registered soon after loading configuration.
The problem is when we run a random test - for example a provider test where Init(config) is not explicitly called. Metrics will be called on nil object and an error will occur. Similarly, logging, which is used across the application, passes through tests without having to explicitly initialize it before each test. And that's because Nop() inits logger silently.
This way, the metric always has a default (empty but not nil) behavior (uses this prefix), and the behavior of the metric can only be changed once by calling metrics.Init(config).
There was a problem hiding this comment.
By calling nop in the var or init section it is guaranteed that the object will be initialized right after the package is loaded, so I don't have to worry about whether the metrics is nil or not
There was a problem hiding this comment.
Very cool, thanks for the explanation!
| if err != nil { | ||
| return err | ||
| } | ||
| m.UpdateIngressHostsPerStatusMetric(gslb, gslb.Status.ServiceHealth) |
There was a problem hiding this comment.
It can be multiple services per Gslb resource. How do we handle that?
There was a problem hiding this comment.
I'm not 100% sure I understand the question.
We are currently using 3 metrics and I wanted to include the expansion of new metrics in the next PR.
- UpdateIngressHostsPerStatusMetric(gslb *k8gbv1beta1.Gslb, serviceHealth map[string]string)
- UpdateHealthyRecordsMetric(gslb *k8gbv1beta1.Gslb, healthyRecords map[string][]string)
- ReconciliationIncrement() - the question here is whether we are interested in the number of reconciliations per controller or per GSLB. It's increment type so metric is number per controller.
- ZoneUpdateIncrement() - this metric is implemented but not used anywhere except in the test; it's incement type.
There was a problem hiding this comment.
@kuritka got it, they are namespace labeled so all good
https://github.com/k8gb-io/k8gb/blob/prometheus_no3_metrics_global_object/controllers/providers/metrics/prometheus.go#L76-L81
related to #124
In this pull request I implemented the metrics system in k8gb. I added one metric - counter
incrementing the number of successful reconciliation loops. Adding specific metrics is part of the next PR.
Like the logger, the metrics are accessed through a single singleton object and like the logger it is initialized.
then anywhere in the code we can use this:
The singleton is initialized in the
main()function using themetrics.Init(*config)function.In case the singleton is not initialized, it will be set to the default value and all metrics will
start with the prefix
k8gb_defaultotherwise they will start with the prefix from the env variableK8GB_NAMESPACE.In addition to the singleton, I have changed the original metrics and added tests at the package level.
In addition to the singleton, I significantly refactored the original metrics and added tests at the package level.
The prometheus metrics implementation uses reflection. In short, I read the collectors structure during the runtime:
Now I know all the types and names. From this I generate the map : .
I will then use the map for Register(), Unregister() and Get().
is the name of the metric and is generated from the name of the structure in
collectors.ReconciliationTotal => for example:
k8gb_gslb_healthy_records(k8gb isK8GB_NAMESPACE)Get()is only for testing purposes - and is public only because controller_tests use metrics.I'll consider deleting tests from controller_tests and leaving only terratests and unit_tests at the package level (96% coverage now).
But that will come in the next PR.
Signed-off-by: kuritka kuritka@gmail.com