Add metrics of gophercloud (POST,UPDATE and DELETE) actions by jichenjc · Pull Request #863 · kubernetes-sigs/cluster-api-provider-openstack

jichenjc · 2021-05-08T02:54:41Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #856

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests

/hold

k8s-ci-robot · 2021-05-08T02:54:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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~~ [jichenjc]

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

sbueringer · 2021-05-11T03:41:41Z

@seanschneeweiss @chrischdi Maybe one of you has some bandwitdh to review? :)

jichenjc · 2021-05-11T03:42:02Z

I can't create a local env (because difference between 0.3.4 and master)
so I used 0.3.4 and code should be same ,the result are something like

capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.005"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.01"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.025"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.05"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.1"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.25"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="0.5"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="1"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="2.5"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="5"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="10"} 17
capo_openstack_api_request_duration_seconds_bucket{request="server_create",le="+Inf"} 17
capo_openstack_api_request_duration_seconds_sum{request="server_create"} 0.0024148109999999998
capo_openstack_api_request_duration_seconds_count{request="server_create"} 17

@sbueringer @hidekazuna please help review , thanks

chrischdi

ObserveRequest always returns the same error which we give to it. So when using it there should be a separate if err != nil part, instead we should use what we had before to not loose context :-)

I think there are lots of more places where we call gophercloud / the OpenStack API to observe metrics.

For example:

I think almost all occurencies can be found using, but ignore the highlighted Extract functions:

for l in $(grep . -r -e "github.com/gophercloud"  | sed -E -e 's/:\t+/;/g' -e 's/"$//g' -e 's/;"/;/g' -e 's/ //g' | grep -v go.sum | grep -v go.mod | grep -v 'test/e2e'); do
  FILE="$(cut -d ";" -f 1 <<< $l)"
  PKG="$(cut -d ";" -f 2 <<< $l | cut -d '"' -f 1 | tr '/' ' ' | awk '{print $NF}')"
  echo "  # $FILE"
  cat $FILE | grep -e "$PKG\.[A-Z][a-zA-Z]+\(" -E -A2 -B2
done

chrischdi · 2021-05-11T05:01:16Z

controllers/openstackcluster_controller.go

 func (r *OpenStackClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
 	log := ctrl.LoggerFrom(ctx)

+	mc := metrics.NewMetricPrometheusContext("snapshot", "create")


I don't think we are doing a snapshot create here :-) also the r.Client.Get is no openstack request?!

it's my debug code ... sorry to forget remove them

chrischdi · 2021-05-11T05:02:49Z

pkg/cloud/services/compute/instance.go

+	if mc.ObserveRequest(err) != nil {
+		return nil, err
+	}
+
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return nil, err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

ObserveRequest just exposes a metric and does return the very same error.

chrischdi · 2021-05-11T05:05:48Z

pkg/cloud/services/compute/instance.go

+	if mc.ObserveRequest(err) != nil {
+		return ports.Port{}, err
+	}
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return ports.Port{}, err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:06:12Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+		mc := metrics.NewMetricPrometheusContext("server", "create")



Suggested change

mc := metrics.NewMetricPrometheusContext("server", "create")

mc := metrics.NewMetricPrometheusContext("server", "create")

chrischdi · 2021-05-11T05:06:23Z

pkg/cloud/services/loadbalancer/loadbalancer.go

+		if mc.ObserveRequest(err) != nil {
+			return err
+		}
 		if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:07:02Z

pkg/cloud/services/networking/floatingip.go

+	if mc.ObserveRequest(err) != nil {
+		return nil, err
+	}
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return nil, err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:07:23Z

pkg/cloud/services/networking/network.go

+	if mc.ObserveRequest(err) != nil {
+		return err
+	}
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:07:39Z

pkg/cloud/services/networking/network.go

+	if mc.ObserveRequest(err) != nil {
+		return nil, err
+	}
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return nil, err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:07:58Z

pkg/cloud/services/networking/router.go

+	if mc.ObserveRequest(err) != nil {
+		return nil, err
+	}
 	if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return nil, err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

chrischdi · 2021-05-11T05:08:34Z

pkg/cloud/services/networking/securitygroups.go

+
+		if mc.ObserveRequest(err) != nil {
+			return err
+		}
 		if err != nil {


Suggested change

if mc.ObserveRequest(err) != nil {

return err

}

if err != nil {

if mc.ObserveRequest(err) != nil {

thanks for the comments, will check other possible metrics

jichenjc · 2021-05-11T08:29:31Z

/test pull-cluster-api-provider-openstack-e2e-test

jichenjc · 2021-05-12T06:30:59Z

@chrischdi I checked again the question you posted above for additional metrics ..
those are GET all which might be overkill , I think POST/PUT is good enough for most customer?

pkg/cloud/services/compute/instance.go

pkg/cloud/services/networking/floatingip.go

chrischdi · 2021-05-12T06:40:57Z

@chrischdi I checked again the question you posted above for additional metrics ..
those are GET all which might be overkill , I think POST/PUT is good enough for most customer?

I think it would be helpful to also have a metric for the get / list requests. It is also done like this in https://github.com/kubernetes/cloud-provider-openstack .

These requests could also fail and another use-case for me would be checking how many requests are done to OpenStack to not overload it.

seanschneeweiss

Thank you for adding all the metrics :) I would also suggest to add metrics for the get/list and delete requests.

pkg/metrics/metrics.go

pkg/cloud/services/compute/instance.go

pkg/cloud/services/loadbalancer/loadbalancer.go

jichenjc · 2021-05-13T00:45:40Z

@chrischdi @seanschneeweiss thanks for the review most comments addressed
I found the List/Get is widely used so I propose to make it next PR to handle (It might need some refactory of code to combe those Get/List into one function..)

sbueringer · 2021-05-25T05:20:42Z

@jichenjc I'll take another look. Let's please merge this ASAP afterwards. With every refactoring PR we merge in the meantime we could introduce new bugs here. So it's a lot of effort to review it all over again to make sure we're safe :/

sbueringer

last round of nits

pkg/cloud/services/compute/instance.go

pkg/cloud/services/loadbalancer/loadbalancer.go

pkg/cloud/services/networking/securitygroups.go

sbueringer · 2021-05-25T05:26:27Z

pkg/cloud/services/networking/securitygroups.go

 		SecGroupID:     r.SecurityGroupID,
 	}
 	s.logger.V(6).Info("Creating rule", "Description", r.Description, "Direction", dir, "PortRangeMin", r.PortRangeMin, "PortRangeMax", r.PortRangeMax, "Proto", proto, "etherType", etherType, "RemoteGroupID", r.RemoteGroupID, "RemoteIPPrefix", r.RemoteIPPrefix, "SecurityGroupID", r.SecurityGroupID)
+	mc := metrics.NewMetricPrometheusContext("security_group_rules", "create")


Suggested change

mc := metrics.NewMetricPrometheusContext("security_group_rules", "create")

mc := metrics.NewMetricPrometheusContext("security_group_rule", "create")

searched and no other plural anymore ..

sbueringer · 2021-05-25T08:31:18Z

/lgtm

jichenjc · 2021-05-25T08:41:31Z

/hold cancel

sbueringer · 2021-05-25T08:49:20Z

/hold

@jichenjc you have to squash locally + push -force before. Otherwise we will get 8 commits on master.

jichenjc · 2021-05-25T08:53:00Z

/hold

@jichenjc you have to squash locally + push -force before. Otherwise we will get 8 commits on master.

my bad, another mistake ... not a lucky day for me

chrischdi · 2021-05-25T08:59:47Z

Squashed :-)

/lgtm

sbueringer · 2021-05-25T09:10:00Z

@jichenjc No problem :).

/lgtm

seanschneeweiss · 2021-05-25T09:32:28Z

/lgtm

Thank you.

jichenjc · 2021-05-25T14:07:01Z

/hold cancel

k8s-ci-robot requested review from hidekazuna and prankul88 May 8, 2021 02:54

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2021

jichenjc force-pushed the add_metrics branch 2 times, most recently from 974ab16 to f306230 Compare May 11, 2021 03:38

jichenjc changed the title ~~WIP:Add metrics~~ Add metrics of gophercloud May 11, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2021

jichenjc force-pushed the add_metrics branch from f306230 to a7ddcbb Compare May 11, 2021 03:52

chrischdi reviewed May 11, 2021

View reviewed changes

jichenjc force-pushed the add_metrics branch 2 times, most recently from d806f20 to 89836fe Compare May 11, 2021 06:25

jichenjc force-pushed the add_metrics branch 4 times, most recently from 740f78d to 516b4e1 Compare May 12, 2021 06:29

chrischdi reviewed May 12, 2021

View reviewed changes

pkg/cloud/services/compute/instance.go Show resolved Hide resolved

pkg/cloud/services/networking/floatingip.go Outdated Show resolved Hide resolved

seanschneeweiss reviewed May 12, 2021

View reviewed changes

jichenjc changed the title ~~Add metrics of gophercloud~~ Add metrics of gophercloud (POST,UPDATE and DELETE) actions May 13, 2021

jichenjc force-pushed the add_metrics branch from 9679295 to a0814e9 Compare May 13, 2021 00:59

jichenjc force-pushed the add_metrics branch from c657503 to 4a86d11 Compare May 25, 2021 00:11

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

jichenjc force-pushed the add_metrics branch from 4a86d11 to 8c563b7 Compare May 25, 2021 01:24

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

jichenjc force-pushed the add_metrics branch 3 times, most recently from 661a547 to 74fec38 Compare May 25, 2021 01:55

sbueringer reviewed May 25, 2021

View reviewed changes

sbueringer mentioned this pull request May 25, 2021

✨ Add feature to create ports with custom options #876

Merged

7 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021

Add metrics of POST/UPDATE/DELETE actions

16c002f

jichenjc force-pushed the add_metrics branch from b485234 to 16c002f Compare May 25, 2021 08:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

k8s-ci-robot assigned chrischdi May 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

k8s-ci-robot assigned seanschneeweiss May 25, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021

k8s-ci-robot merged commit cb38623 into kubernetes-sigs:master May 25, 2021

jichenjc mentioned this pull request Jul 8, 2021

Expose gophercloud requests metrics #856

Closed

		mc := metrics.NewMetricPrometheusContext("server", "create")

Conversation

jichenjc commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented May 8, 2021

Uh oh!

sbueringer commented May 11, 2021

Uh oh!

jichenjc commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrischdi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jichenjc commented May 11, 2021

Uh oh!

jichenjc commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

chrischdi commented May 12, 2021

Uh oh!

seanschneeweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jichenjc commented May 13, 2021

Uh oh!

sbueringer commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer commented May 25, 2021

Uh oh!

jichenjc commented May 25, 2021

Uh oh!

sbueringer commented May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jichenjc commented May 8, 2021 •

edited

Loading

jichenjc commented May 11, 2021 •

edited

Loading

sbueringer commented May 25, 2021 •

edited

Loading

sbueringer commented May 25, 2021 •

edited

Loading