chore(metrics): refactor metrics to use common registry

[mwmix]

What does it do ?

This PR refactors the metrics so that the request_duration_metric is configured in the same way as the other metrics.

Motivation

While working on solving the AWS_CA_BUNDLE issue we had to refactor the request_duration_metric and it was decided to split the refactor off into it's own PR.

More

• [ x ] Yes, this PR title follows Conventional Commits
• [ x ] Yes, I added unit tests
• [ x ] Yes, I updated end user documentation accordingly

[k8s-ci-robot]

Hi @mwmix. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes-sigs/prow repository.

[ivankatliarchuk]

/ok-to-test

[ivankatliarchuk]

Any chance to share that the metric is still works. It should capture calls to AWS endpionts, example screenshot is here #5650 (comment)

[mwmix]

Will just a copy pasted console output work?

❯ curl http://localhost:7979/metrics | grep -i request_
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 15009    0 15009    0     0  73794      # HELP external_dns_http_request_duration_seconds The HTTP request latencies in seconds.
0 -# TYPE external_dns_http_request_duration_seconds summary
-external_dns_http_request_duration_seconds{handler="instrumented_http",host="route53.amazonaws.com",method="get",path="hostedzone",scheme="https",status="200",quantile="0.5"} 0.078072332
:external_dns_http_request_duration_seconds{handler="instrumented_http",host="route53.amazonaws.com",method="get",path="hostedzone",scheme="https",status="200",quantile="0.9"} 3.089020648
-external_dns_http_request_duration_seconds{handler="instrumented_http",host="route53.amazonaws.com",method="get",path="hostedzone",scheme="https",status="200",quantile="0.99"} 3.089020648
-external_dns_http_request_duration_seconds_sum{handler="instrumented_http",host="route53.amazonaws.com",method="get",path="hostedzone",scheme="https",status="200"} 3.198551555
:external_dns_http_request_duration_seconds_count{handler="instrumented_http",host="route53.amazonaws.com",method="get",path="hostedzone",scheme="https",status="200"} 3
-- --:--:-- --:--:-- 73935

The name of the metric is now prefixed with "external_dns_". Did you want me to try to remove that? ( I'm not sure I can, but I've also not tried ).

[ivankatliarchuk]

May need to double check KubeAPI metrics (should not be affected, just in case), every time there is a request to kubeAPI same metrics captured. Instrumentation

external-dns/source/store.go

Line 628 in 20ab886

config.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {

[ivankatliarchuk]

I did some research, this is a default client. We could keep it, to make it explicit, more a question where or not it will work correctly without it

[mwmix]

Yeah you are right we don't need it. It works fine if we just use the default client. I made that update over on the other PR since it seemed appropriate and am rebasing it into here.

[ivankatliarchuk]

We lost type safety here. Before it was labels Labels, now value float64, lvs ...string.

[ivankatliarchuk]

maybe is possible to have a type safety? If not, let's leave it

[mwmix]

I modeled my SetWithLabels function to work the same way as the GaugeVecMetric's SetWithLabels():

// SetWithLabels sets the value of the Gauge metric for the specified label values.
// All label values are converted to lowercase before being applied.
func (g GaugeVecMetric) SetWithLabels(value float64, lvs ...string) {
	for i, v := range lvs {
		lvs[i] = strings.ToLower(v)
	}
	g.Gauge.WithLabelValues(lvs...).Set(value)
}

In both cases we could modify the function to take prometheus.Labels in but ultimately we would still have to convert into []string for the WithLabelValues() call at the end.

[ivankatliarchuk]

Ok.

There is paralel works happehing in go library prometheus/client_golang#1801. So I hope we have native solution soonish prometheus/client_golang#1801

[ivankatliarchuk]

I though you planning to slice pr and first update metrics itself. Not sure how is differ to othe PR. But anyway, the code coverage needs increasing to 80-90% if possible for new code. Error cases quite important too

[mwmix]

I updated #5665 so that it is just the AWS fix.

As for this PR I've added some more tests. Is what I've added sufficient? Or do I need to add more?

[ivankatliarchuk]

this will crash external-dns. Would rather review this in follow-up PR

[mwmix]

I've removed the else block so it won't crash anymore.

[ivankatliarchuk]

May need to double check KubeAPI metrics (should not be affected, just in case), every time there is a request to kubeAPI same metrics captured. Instrumentation

external-dns/source/store.go

Line 628 in 20ab886

config.WrapTransport = func(rt http.RoundTripper) http.RoundTripper {

[ivankatliarchuk]

Let's make it simple. The PR should not contain any changes in provider/aws.

Order

1. This PR, only changes in metrics
2. PR fix(aws): support aws_ca_bundle #5665 with CA bundle support

[mwmix]

Let's make it simple. The PR should not contain any changes in provider/aws.

Order

1. This PR, only changes in metrics
2. PR fix(aws): support aws_ca_bundle #5665 with CA bundle support

Okay so both PRs have been updated. Let me know what you would like to me to check or fix.

[ivankatliarchuk]

/lgtm

cc: @mloiseleur for final review

[ivankatliarchuk]

Ok.

There is paralel works happehing in go library prometheus/client_golang#1801. So I hope we have native solution soonish prometheus/client_golang#1801

[ivankatliarchuk]

Just a memo. When ported this method, not added any test coverage

TODO: in follow up

[mwmix]

I've gone and created a PR that adds coverage for this method and a couple others that were missing it #5712

[mloiseleur]

@mwmix PR lgtm. Would you please rebase it ? We should be able to merge it afterwards.

[mwmix]

@mwmix PR lgtm. Would you please rebase it ? We should be able to merge it afterwards.

@mloiseleur Done.

[ivankatliarchuk]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivankatliarchuk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [ivankatliarchuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment