Instrument metrics #645

cerberus20 · 2024-02-29T05:03:16Z

linux-foundation-easycla · 2024-02-29T05:03:20Z

The committers listed above are authorized under a signed CLA.

✅ login: cerberus20 (2ba4422, 57b07c9, b01d7ac, 118b2fe, e70cb42, abe484a, 34c2e3e, 28cd9fd, 00c0930, 4893491, fdff124, 8480eef)

k8s-ci-robot · 2024-02-29T05:03:25Z

Hi @cerberus20. 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.

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/test-infra repository.

dashpole · 2024-03-07T17:46:22Z

/assign @dgrisonnet
/triage accepted

k8s-ci-robot · 2024-03-27T10:49:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cerberus20
Once this PR has been reviewed and has the lgtm label, please ask for approval from dgrisonnet. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

cerberus20 · 2024-03-27T10:52:59Z

@dgrisonnet /ping

cerberus20 · 2024-04-15T03:14:57Z

@dgrisonnet Can you please help review? thank you.

dgrisonnet · 2024-05-03T12:38:11Z

pkg/client/metrics/metrics.go

+	apiErrorCount = metrics.NewCounterVec(
+    		&metrics.CounterOpts{
+        		Namespace: "prometheus_adapter",
+        		Subsystem: "prometheus_client",
+        		Name:      "api_errors_total",
+        		Help:      "Total number of API errors",
+    		},
+    		[]string{"error_code", "path", "server"},


In order to compute an error rate and produce an alert out of this metric, it is usually better to record all the request including the one that succeed.

A common practice is to have a http_requests_total metric with a code label. In you case I would suggest to have a api_requests_total metric that record all the requests and rename the label to code and set it to the http status code of the request.

ack, makes sense, updated with api_requests_total. thanks for the suggestion.

dgrisonnet · 2024-05-03T12:39:29Z

pkg/client/metrics/metrics.go

+	errRegisterQueryLatency := registry.Register(queryLatency)
+	if errRegisterQueryLatency != nil {
+		return nil, errRegisterQueryLatency
+	}
+
+	errRegisterAPIErrorCount := registry.Register(apiErrorCount)
+	if errRegisterAPIErrorCount != nil {
+		return nil, errRegisterAPIErrorCount
 	}


Since we don't necessarily need to differentiate between the errors here, it is usually a common practice in Go to just have one err variable and reuse it for reach Register call.

updated, thanks

dgrisonnet · 2024-05-16T17:55:25Z

pkg/client/metrics/metrics.go

+			if apiErr, wasAPIErr := err.(*client.Error); wasAPIErr {
+				apiRequestsTotal.With(prometheus.Labels{"code": string(apiErr.Type), "path": endpoint, "server": c.serverName}).Inc()
+			} else {
+				apiRequestsTotal.With(prometheus.Labels{"code": "generic", "path": endpoint, "server": c.serverName}).Inc()
 			}


When I first looked at your PR I had forgotten that the client errors were wrapped https://github.com/kubernetes-sigs/prometheus-adapter/blob/master/pkg/client/types.go#L27-L31 which is not ideal as it is less explicit to users than a normal http code would be. I feel like we should change some things in order to surface the http code and expose it in the metrics.

I'll be out till the 27th of May, but I'll try to have a look at that again when I am back.

@dgrisonnet I updated code to add HTTPStatusCode to APIResponse to expose the http code in the metric. Can you please review? Thank you.

…ent-metrics # Conflicts: # pkg/client/metrics/metrics.go

k8s-triage-robot · 2024-09-15T06:11:35Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-10-15T06:52:25Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

cerberus20 added 5 commits February 22, 2024 11:44

Adding extra metric till now, not impacting path

e70cb42

add telemetry endpoint

57b07c9

instrument metrics

2ba4422

assign /metrics to metrics handler

118b2fe

revert makefile changes

abe484a

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 29, 2024

k8s-ci-robot requested review from dgrisonnet and olivierlemasle February 29, 2024 05:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 29, 2024

k8s-ci-robot assigned dgrisonnet Mar 7, 2024

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 7, 2024

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

Merge branch 'master' into instrument-metrics

28cd9fd

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

remove extra space

4893491

dgrisonnet reviewed May 3, 2024

View reviewed changes

cerberus20 and others added 2 commits May 3, 2024 19:32

update error metric to api requests total

00c0930

Update comments

fdff124

dgrisonnet reviewed May 16, 2024

View reviewed changes

cerberus20 added 3 commits June 17, 2024 11:21

add http status code to APIResponse

b01d7ac

Merge remote-tracking branch 'origin/instrument-metrics' into instrum…

34c2e3e

…ent-metrics # Conflicts: # pkg/client/metrics/metrics.go

add http status code to APIResponse

8480eef

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 15, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument metrics #645

Instrument metrics #645

cerberus20 commented Feb 29, 2024

linux-foundation-easycla bot commented Feb 29, 2024 •

edited

Loading

k8s-ci-robot commented Feb 29, 2024

dashpole commented Mar 7, 2024

k8s-ci-robot commented Mar 27, 2024

cerberus20 commented Mar 27, 2024

cerberus20 commented Apr 15, 2024

dgrisonnet May 3, 2024

cerberus20 May 3, 2024

dgrisonnet May 3, 2024

cerberus20 May 3, 2024

dgrisonnet May 16, 2024

cerberus20 Jun 17, 2024

k8s-triage-robot commented Sep 15, 2024

k8s-triage-robot commented Oct 15, 2024

Instrument metrics #645

Are you sure you want to change the base?

Instrument metrics #645

Conversation

cerberus20 commented Feb 29, 2024

linux-foundation-easycla bot commented Feb 29, 2024 • edited Loading

k8s-ci-robot commented Feb 29, 2024

dashpole commented Mar 7, 2024

k8s-ci-robot commented Mar 27, 2024

cerberus20 commented Mar 27, 2024

cerberus20 commented Apr 15, 2024

dgrisonnet May 3, 2024

Choose a reason for hiding this comment

cerberus20 May 3, 2024

Choose a reason for hiding this comment

dgrisonnet May 3, 2024

Choose a reason for hiding this comment

cerberus20 May 3, 2024

Choose a reason for hiding this comment

dgrisonnet May 16, 2024

Choose a reason for hiding this comment

cerberus20 Jun 17, 2024

Choose a reason for hiding this comment

k8s-triage-robot commented Sep 15, 2024

k8s-triage-robot commented Oct 15, 2024

linux-foundation-easycla bot commented Feb 29, 2024 •

edited

Loading