Update kubevirt_rest_client_request_latency_seconds to count list cal… #12716

Sreeja1725 · 2024-09-02T13:59:14Z

What this PR does

Currently, when a list call is made using field-selector or label-selector those calls are inaccurately counted as get calls. This MR will solve this issue.

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

 Update kubevirt_rest_client_request_latency_seconds to count list calls if made using query params

kubevirt-bot · 2024-09-02T13:59:23Z

Hi @Sreeja1725. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

Sreeja1725 · 2024-09-02T14:02:48Z

cc @alaypatel07

xpivarc · 2024-09-03T12:06:40Z

/cc @machadovilaca

machadovilaca · 2024-09-03T12:19:00Z

thanks for the PR @Sreeja1725, we would need to validate this change (removing the usage of the latency adapter for requestLatency) does not have unexpected consequences. I wonder if you couldn't adapt

kubevirt/pkg/monitoring/metrics/common/client/latency_adapter.go

Line 33 in 7213150

 func (l *latencyAdapter) Observe(_ context.Context, verb string, u url.URL, latency time.Duration) { 

instead, since you have access to the URL there too

Sreeja1725 · 2024-09-03T15:53:20Z

thanks for the PR @Sreeja1725, we would need to validate this change (removing the usage of the latency adapter for requestLatency) does not have unexpected consequences. I wonder if you couldn't adapt

kubevirt/pkg/monitoring/metrics/common/client/latency_adapter.go

Line 33 in 7213150

func (l *latencyAdapter) Observe(_ context.Context, verb string, u url.URL, latency time.Duration) {

instead, since you have access to the URL there too

Thanks for your response @machadovilaca , I have updated as per your comment. PTAL

machadovilaca · 2024-09-03T16:04:20Z

pkg/monitoring/metrics/common/client/latency_adapter.go

+ resource := findResource(u)
+ if resource == "" {
+ resource = "none"
+ }


I see that you improved a bit, was going to comment on this, but what would you think about:

func getVerbLabelValue(u url.URL, verb string) string { if verb != "GET" { return verb } if strings.Contains(u.Path, "/watch/") || u.Query().Get("watch") == "true" { return "WATCH" } resource := findResource(u) if resource == "" { resource = "none" } if strings.HasSuffix(u.Path, resource) { // If the resource is the last element in the url, then // we're asking to list all resources of that type instead // of getting an individual resource return "LIST" } return verb }

yet a bit better, no?

and then just
l.m.WithLabelValues(getVerbLabelValue(u, verb), u.String()).Observe(latency.Seconds())

Yes, it's better! The refactoring made the code cleaner and modular. Thanks @machadovilaca for simplifying the logic and separating concerns.

alaypatel07 · 2024-09-09T15:41:56Z

/ok-to-test

alaypatel07 · 2024-09-09T16:42:34Z

/retest

alaypatel07 · 2024-09-09T16:43:36Z

@machadovilaca can you please do a second round of reviews on this PR?

machadovilaca · 2024-09-19T12:43:08Z

/approve

kubevirt-bot · 2024-09-19T12:43:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: machadovilaca

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/monitoring/OWNERS~~ [machadovilaca]

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

Sreeja1725 · 2024-09-26T06:27:15Z

@xpivarc Can you PTAL

xpivarc · 2024-09-26T12:15:15Z

/cc @0xFelix

0xFelix

Added some comments can you please put something in the commit message?

0xFelix · 2024-09-26T12:35:08Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ resource := findResource(u)
+ if resource == "" {
+ return "none"
+ }


Could be inlined

0xFelix · 2024-09-26T12:35:26Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ match := r.FindStringSubmatch(u.Path)
+ if len(match) > 1 {


Could be inlined

0xFelix · 2024-09-26T12:36:21Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ }
+ match := r.FindStringSubmatch(u.Path)
+ if len(match) > 1 {
+ resource = match[1]


Suggested change

resource = match[1]

return match[1]

0xFelix · 2024-09-26T12:36:26Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ if resource != "" {
+ break
+ }


0xFelix · 2024-09-26T12:36:37Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ resource = match[1]
+ }
+ }
+ return resource


Suggested change

return resource

return ""

0xFelix · 2024-09-26T12:37:38Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ return resource, verb
+}
+
+func determineVerb(u url.URL, methodOrVerb string) (verb string) {


Can you make the funcs name more descriptive? Something like getVerbFromHTTPVerb?

0xFelix · 2024-09-26T12:38:47Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ return resource
+}
+
+func getHost(request *http.Request) string {


Is it used only in a single place? If so it's okay to keep it inline.

0xFelix · 2024-09-26T12:40:11Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

+ resource string
+ verb string
+ host string


Define these vars on the go with :=

Sreeja1725 · 2024-09-26T17:28:32Z

@0xFelix Addressed all your comments, PTAL

0xFelix

/lgtm

Two nits, feel free to address

0xFelix · 2024-09-30T11:33:19Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

 }

- switch method {
+ verb = getVerbFromHTTPVerb(*request.URL, method)


nit: Could be inlined

0xFelix · 2024-09-30T11:33:49Z

pkg/monitoring/metrics/common/client/rt_wrapper.go

- var resource string
- var verb string
- var host string
+ var status, host string


Suggested change

var status, host string

var status string

var host string

nit: Not sure we got a rule for that though

kubevirt-commenter-bot · 2024-09-30T11:34:35Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

…econds Signed-off-by: svarnam <[email protected]>

Sreeja1725 · 2024-09-30T14:18:55Z

@0xFelix Updated as per your comments, PTAL

0xFelix

/lgtm

Thanks!

kubevirt-commenter-bot · 2024-10-01T09:12:43Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

kubevirt-bot requested a review from assafad September 2, 2024 13:59

kubevirt-bot added the dco-signoff: no Indicates the PR's author has not DCO signed all their commits. label Sep 2, 2024

kubevirt-bot requested a review from avlitman September 2, 2024 13:59

kubevirt-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/observability Denotes an issue or PR that relates to observability. labels Sep 2, 2024

kubevirt-bot added the size/XS label Sep 2, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from 3b6d13c to 7616b52 Compare September 2, 2024 14:01

Sreeja1725 marked this pull request as draft September 2, 2024 17:42

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 2, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from 7616b52 to 7213150 Compare September 3, 2024 07:32

kubevirt-bot requested a review from machadovilaca September 3, 2024 12:06

Sreeja1725 force-pushed the update_metric_rest_client branch from 7213150 to 7fe8be0 Compare September 3, 2024 15:50

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/M and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XS labels Sep 3, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from 7fe8be0 to c5049b8 Compare September 3, 2024 15:52

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 3, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from c5049b8 to d10276e Compare September 3, 2024 15:55

machadovilaca reviewed Sep 3, 2024

View reviewed changes

dosubot bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Sep 9, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 19, 2024

kubevirt-bot requested a review from 0xFelix September 26, 2024 12:15

0xFelix reviewed Sep 26, 2024

View reviewed changes

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 26, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from 87e5ec4 to 9ce9c06 Compare September 26, 2024 15:35

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 26, 2024

Sreeja1725 force-pushed the update_metric_rest_client branch from 9ce9c06 to 35fe879 Compare September 26, 2024 17:26

0xFelix reviewed Sep 30, 2024

View reviewed changes

kubevirt-bot assigned 0xFelix Sep 30, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2024

Accurately count list calls in kubevirt_rest_client_request_latency_s…

b4cb1b4

…econds Signed-off-by: svarnam <[email protected]>

Sreeja1725 force-pushed the update_metric_rest_client branch from 35fe879 to b4cb1b4 Compare September 30, 2024 14:17

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2024

0xFelix approved these changes Oct 1, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2024

kubevirt-bot merged commit ecc7c8f into kubevirt:main Oct 1, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update kubevirt_rest_client_request_latency_seconds to count list cal… #12716

Update kubevirt_rest_client_request_latency_seconds to count list cal… #12716

Sreeja1725 commented Sep 2, 2024 •

edited

Loading

kubevirt-bot commented Sep 2, 2024

Sreeja1725 commented Sep 2, 2024

xpivarc commented Sep 3, 2024

machadovilaca commented Sep 3, 2024

Sreeja1725 commented Sep 3, 2024

machadovilaca Sep 3, 2024

machadovilaca Sep 3, 2024

Sreeja1725 Sep 4, 2024

alaypatel07 commented Sep 9, 2024

alaypatel07 commented Sep 9, 2024

alaypatel07 commented Sep 9, 2024

machadovilaca commented Sep 19, 2024

kubevirt-bot commented Sep 19, 2024

Sreeja1725 commented Sep 26, 2024

xpivarc commented Sep 26, 2024

0xFelix left a comment

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

0xFelix Sep 26, 2024

Sreeja1725 commented Sep 26, 2024

0xFelix left a comment

0xFelix Sep 30, 2024

0xFelix Sep 30, 2024

kubevirt-commenter-bot commented Sep 30, 2024

Sreeja1725 commented Sep 30, 2024

0xFelix left a comment

kubevirt-commenter-bot commented Oct 1, 2024

Update kubevirt_rest_client_request_latency_seconds to count list cal… #12716

Update kubevirt_rest_client_request_latency_seconds to count list cal… #12716

Conversation

Sreeja1725 commented Sep 2, 2024 • edited Loading

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented Sep 2, 2024

Sreeja1725 commented Sep 2, 2024

xpivarc commented Sep 3, 2024

machadovilaca commented Sep 3, 2024

Sreeja1725 commented Sep 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alaypatel07 commented Sep 9, 2024

alaypatel07 commented Sep 9, 2024

alaypatel07 commented Sep 9, 2024

machadovilaca commented Sep 19, 2024

kubevirt-bot commented Sep 19, 2024

Sreeja1725 commented Sep 26, 2024

xpivarc commented Sep 26, 2024

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sreeja1725 commented Sep 26, 2024

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Sep 30, 2024

Sreeja1725 commented Sep 30, 2024

0xFelix left a comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Oct 1, 2024

Sreeja1725 commented Sep 2, 2024 •

edited

Loading