Proxy sli #342

ranakan19 · 2023-09-01T12:15:18Z

This PR introduces two metrics for Registration-Service proxy:

RegServProxyApiHistogramVec - measures the time taken by proxy before forwarding the request
RegServWorkspaceHistogramVec - measures the response time for either response or error from proxy when there is no routing

Since they are both of the type HistogramVec, the metrics can be partioned on labels.
RegServWorkspaceHistogramVec uses labels status_code and kube_verb to identify the type of request() and whether the request was successful.
While RegServProxyRouteHistogramVec uses labels status_code and route_to to store Host URL of the target cluster and whether the request was acceptable. In the case that the request wasn't acceptable the value for route_to will be populated as Rejected

Here is a sample of the output of metrics on dev-cluster:

# HELP sandbox_proxy_api_http_request_time time taken by proxy to route to a target cluster
# TYPE sandbox_proxy_api_http_request_time histogram
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="0.05"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="0.1"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="0.25"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="0.5"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="1"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="5"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="10"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="Rejected",status_code="406",le="+Inf"} 1
sandbox_proxy_api_http_request_time_sum{route_to="Rejected",status_code="406"} 0.000216156
sandbox_proxy_api_http_request_time_count{route_to="Rejected",status_code="406"} 1
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="0.05"} 12
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="0.1"} 12
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="0.25"} 12
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="0.5"} 12
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="1"} 12
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="5"} 13
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="10"} 13
sandbox_proxy_api_http_request_time_bucket{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202",le="+Inf"} 13
sandbox_proxy_api_http_request_time_sum{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202"} 2.5381906469999995
sandbox_proxy_api_http_request_time_count{route_to="api.krana-sep2723.devcluster.openshift.com:6443",status_code="202"} 13
# HELP sandbox_proxy_workspace_http_request_time time for response of a request to proxy 
# TYPE sandbox_proxy_workspace_http_request_time histogram
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="0.05"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="0.1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="0.25"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="0.5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="10"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="200",le="+Inf"} 1
sandbox_proxy_workspace_http_request_time_sum{kube_verb="Get",status_code="200"} 0.000774987
sandbox_proxy_workspace_http_request_time_count{kube_verb="Get",status_code="200"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="0.05"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="0.1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="0.25"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="0.5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="10"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="Get",status_code="404",le="+Inf"} 1
sandbox_proxy_workspace_http_request_time_sum{kube_verb="Get",status_code="404"} 0.00072671
sandbox_proxy_workspace_http_request_time_count{kube_verb="Get",status_code="404"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="0.05"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="0.1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="0.25"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="0.5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="10"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="200",le="+Inf"} 1
sandbox_proxy_workspace_http_request_time_sum{kube_verb="List",status_code="200"} 0.002846301
sandbox_proxy_workspace_http_request_time_count{kube_verb="List",status_code="200"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="0.05"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="0.1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="0.25"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="0.5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="1"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="5"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="10"} 1
sandbox_proxy_workspace_http_request_time_bucket{kube_verb="List",status_code="500",le="+Inf"} 1
sandbox_proxy_workspace_http_request_time_sum{kube_verb="List",status_code="500"} 0.000815715
sandbox_proxy_workspace_http_request_time_count{kube_verb="List",status_code="500"} 1

openshift-ci · 2023-09-01T12:15:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

…ues in test

ranakan19 · 2023-09-01T23:26:41Z

/retest

pkg/proxy/proxy.go

MatousJobanek

overall looks good 👍 I added a few suggestions, but nothing critical.
Could you please update also the unit tests?

pkg/context/keys.go

pkg/metrics/metrics.go

pkg/proxy/handlers/spacelister.go

pkg/proxy/proxy.go

pkg/metrics/metrics.go

codecov · 2023-09-14T19:06:04Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
pkg/proxy/handlers/metrics.go	`100.00% <100.00%> (ø)`
pkg/proxy/handlers/spacelister.go	`83.05% <100.00%> (+0.59%)`	⬆️
pkg/server/routes.go	`77.77% <ø> (-0.49%)`	⬇️
pkg/proxy/proxy.go	`85.26% <93.75%> (+0.77%)`	⬆️
pkg/metrics/metrics.go	`88.00% <88.00%> (ø)`

📢 Thoughts on this report? Let us know!.

rajivnathan

Looks good overall! Just a few questions in the comments.

pkg/metrics/metrics_test.go

pkg/proxy/handlers/spacelister.go

ranakan19 · 2023-09-19T15:24:49Z

/retest

MatousJobanek

Just a few reminders

pkg/context/keys.go

pkg/metrics/metrics.go

pkg/proxy/proxy.go

pkg/metrics/metrics.go

pkg/proxy/handlers/spacelister.go

pkg/metrics/metrics.go

xcoulon · 2023-09-26T08:24:41Z

pkg/metrics/metrics.go

+	// RegServProxyRouteHistogramVec measures the time taken by proxy before forwarding the request
+	RegServProxyRouteHistogramVec *prometheus.HistogramVec
+	// RegServProxyResponseHistogramVec measures the response time for either response or error from proxy when there is no routing
+	RegServProxyResponseHistogramVec *prometheus.HistogramVec


You could make the variable names shorter by dropping the RegServ prefix:

Suggested change

// RegServProxyRouteHistogramVec measures the time taken by proxy before forwarding the request

RegServProxyRouteHistogramVec *prometheus.HistogramVec

// RegServProxyResponseHistogramVec measures the response time for either response or error from proxy when there is no routing

RegServProxyResponseHistogramVec *prometheus.HistogramVec

// ProxyRouteHistogramVec measures the time taken by proxy before forwarding the request

ProxyRouteHistogramVec *prometheus.HistogramVec

// ProxyResponseHistogramVec measures the response time from proxy when there is no routing

ProxyResponseHistogramVec *prometheus.HistogramVec

pkg/proxy/proxy.go

pkg/server/routes.go

sonarcloud · 2023-09-27T23:51:37Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
12.0% Duplication

xcoulon

great work @ranakan19 ! 🙌
and thanks for addressing my comments 😄

MatousJobanek

Nice 👍 Good job @ranakan19 🥇 🚀

rajivnathan

Great work! 🙌

rajivnathan · 2023-09-28T14:04:50Z

pkg/metrics/metrics_test.go

+	m.WithLabelValues("500", "list").Observe((1 * time.Millisecond).Seconds())
+
+	//then
+	assert.Equal(t, 4, promtestutil.CollectAndCount(m, "sandbox_test_histogram_vec"))


Just a question, why is it 4? Maybe worth adding a comment

because HistogramVec are partioned on labels, and we have 4 label combination in the test. Sure let me add a label

openshift-ci · 2023-09-28T14:11:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MatousJobanek, rajivnathan, ranakan19, xcoulon

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:

~~OWNERS~~ [MatousJobanek,xcoulon]

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

ranakan19 added 4 commits August 29, 2023 09:01

groundwork for registration route metrics

45fde62

expose metrics along with other routes

7be0d8d

add response metrics

8f57388

remove echo middleware

1eb0678

openshift-ci bot added the do-not-merge/work-in-progress label Sep 1, 2023

openshift-merge-robot added the needs-rebase label Sep 1, 2023

ranakan19 and others added 5 commits September 1, 2023 16:10

add header, edit handler

7a21847

rename vectors, and update comments

92881f9

remove commented code

dec95fd

keep only approve and reject labels for response, compare metrics val…

3d91ec2

…ues in test

Merge branch 'master' into proxy-sli

db4caeb

openshift-merge-robot removed the needs-rebase label Sep 1, 2023

mod tidy

571ad09

ranakan19 marked this pull request as ready for review September 1, 2023 23:26

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 1, 2023

openshift-ci bot requested review from michaelkleinhenz and sbryzak September 1, 2023 23:26

ranakan19 requested review from xcoulon, alexeykazakov, MatousJobanek, mfrancisc and rajivnathan and removed request for michaelkleinhenz September 1, 2023 23:26

alexeykazakov reviewed Sep 1, 2023

View reviewed changes

pkg/proxy/proxy.go Outdated Show resolved Hide resolved

ranakan19 and others added 3 commits September 1, 2023 20:16

add starttime to context

79447b8

Merge branch 'master' into proxy-sli

d2207aa

lint check, remove unused lib from imports

4b2d869

MatousJobanek reviewed Sep 14, 2023

View reviewed changes

extract processing from handling request

50d5080

openshift-merge-robot added the needs-rebase label Sep 14, 2023

Merge branch 'master' into proxy-sli

b77cd5f

openshift-merge-robot removed the needs-rebase label Sep 14, 2023

add metricsController unit test

a72c6ef

rajivnathan reviewed Sep 14, 2023

View reviewed changes

ranakan19 added 2 commits September 18, 2023 21:43

PR review suggestions added to unit test

27cdae3

disable compression

39f970d

MatousJobanek reviewed Sep 21, 2023

View reviewed changes

pkg/context/keys.go Outdated Show resolved Hide resolved

pkg/metrics/metrics.go Outdated Show resolved Hide resolved

pkg/proxy/proxy.go Outdated Show resolved Hide resolved

pkg/metrics/metrics.go Outdated Show resolved Hide resolved

rajivnathan reviewed Sep 21, 2023

View reviewed changes

pkg/proxy/handlers/spacelister.go Outdated Show resolved Hide resolved

ranakan19 added 4 commits September 21, 2023 14:25

move exposing metrics to proxy, instead of reg-service

cc7986c

label changes, bucket changes, handler test

76a6356

correct spelling

832dfda

remove metrics testsuite, and fix lint errors

921b33f

xcoulon requested changes Sep 26, 2023

View reviewed changes

ranakan19 and others added 6 commits September 26, 2023 16:07

add back return param on handler, add nolint for it

5769225

rename metrics to the discussed names with labels

12580c1

typos, leftover comments, rename startTime

631fdc6

add unit tests for comparing samplecount for each label combination

f107e12

format and lint check

50b8d17

Merge branch 'master' into proxy-sli

4bc98ad

xcoulon approved these changes Sep 28, 2023

View reviewed changes

openshift-ci bot added the approved label Sep 28, 2023

MatousJobanek approved these changes Sep 28, 2023

View reviewed changes

rajivnathan approved these changes Sep 28, 2023

View reviewed changes

MatousJobanek merged commit e1dbff6 into codeready-toolchain:master Sep 29, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proxy sli #342

Proxy sli #342

ranakan19 commented Sep 1, 2023 •

edited

Loading

openshift-ci bot commented Sep 1, 2023

ranakan19 commented Sep 1, 2023

MatousJobanek left a comment

codecov bot commented Sep 14, 2023 •

edited

Loading

rajivnathan left a comment

ranakan19 commented Sep 19, 2023

MatousJobanek left a comment

xcoulon Sep 26, 2023

sonarcloud bot commented Sep 27, 2023

xcoulon left a comment

MatousJobanek left a comment

rajivnathan left a comment

rajivnathan Sep 28, 2023

ranakan19 Sep 28, 2023

openshift-ci bot commented Sep 28, 2023

Proxy sli #342

Proxy sli #342

Conversation

ranakan19 commented Sep 1, 2023 • edited Loading

openshift-ci bot commented Sep 1, 2023

ranakan19 commented Sep 1, 2023

MatousJobanek left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 14, 2023 • edited Loading

Codecov Report

rajivnathan left a comment

Choose a reason for hiding this comment

ranakan19 commented Sep 19, 2023

MatousJobanek left a comment

Choose a reason for hiding this comment

xcoulon Sep 26, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Sep 27, 2023

xcoulon left a comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

rajivnathan left a comment

Choose a reason for hiding this comment

rajivnathan Sep 28, 2023

Choose a reason for hiding this comment

ranakan19 Sep 28, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Sep 28, 2023

ranakan19 commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 14, 2023 •

edited

Loading