metrics: add /metrics endpoint and console_helm_install_count metric by zonggen · Pull Request #10194 · openshift/console

zonggen · 2021-10-06T20:36:51Z

Works with openshift/console-operator#601 to add
a console_helm_install_count counter vector metric. This change will
increment the counter each time a user installs a Helm chart in the
console.

Closes: https://issues.redhat.com/browse/HELM-235
Signed-off-by: Allen Bai abai@redhat.com

kyoto · 2021-10-07T00:18:56Z

FYI @openshift/openshift-team-monitoring

zonggen · 2021-10-07T15:47:34Z

Ready for review @dperaza4dustbit.

simonpasquier · 2021-10-08T07:36:28Z

Suggested change

Help: "Number of Helm installation from console.",

Help: "Number of Helm installations from console broken down by chart and version.",

simonpasquier · 2021-10-08T07:43:25Z

AFAIK the only way this function would panic is if you pass a wrong number of labels to WithLabelValues() which would mean that the code is broken somehow.
It's better to use GetMetricWithLabelValues or GetMetricWith() and check the returned error.

zonggen · 2021-10-08T14:31:18Z

@simonpasquier Thanks for reviewing! Updated according to your comments.

dperaza4dustbit

/lgtm

jhadvig

@zonggen thanks for the PR. Adding couple of comments:
Also could you please squash your PR afterwards, at least to two commits - code changes & vendor.
Thanks

jhadvig · 2021-10-13T08:33:05Z

This should not be removed

Added back.

jhadvig · 2021-10-13T08:43:51Z

lets make this Names consts here and use them here and in the metrics_test.go

jhadvig · 2021-10-13T08:45:26Z

lets make these a consts as well and use them across the pkg

jhadvig · 2021-10-13T08:52:18Z

not a native speaker but 'broken down by' sounds a bit odd to me
maybe?

Suggested change

Help: "Number of Helm installations from console broken down by chart name and version.",

Help: "Number of Helm installations from console, based on chart name and version.",

Number of Helm installations from console by chart name and version. should be good

Went with Number of Helm installations from console by chart name and version., please let me know if that sounds okay @jhadvig

jhadvig · 2021-10-13T09:03:14Z

this wont recover the function since the GetMetricWithLabelValues throws a panic.
Instead

func HandleconsoleHelmInstallsTotal(chartName, chartVersion string) { defer recoverMetricPanic() klog.V(4).Infof("metric console_helm_installs_total: %s %s", chartName, chartVersion) counter, _ := consoleHelmInstallsTotal.GetMetricWithLabelValues(chartName, chartVersion) counter.Add(1) } // We will never want to panic console server because of metric saving. // Therefore, we will recover our panics here and error log them // for later diagnosis but will never fail the console server. func recoverMetricPanic() { if r := recover(); r != nil { klog.Errorf("Recovering from metric function - %v", r) } }

We should do the same for the HandleconsoleHelmUpgradesTotal & HandleconsoleHelmUninstallsTotal functions

From the official doc it seems like WithLabelValues will panic in the case when GetMetricWithLabelValues returns an error: https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#CounterVec.GetMetricWithLabelValues vs. https://pkg.go.dev/github.com/prometheus/client_golang/prometheus#CounterVec.WithLabelValues.

Could you confirm the above doc and GetMetricWithLabelValues will indeed panic? Will proceed to update after confirming :)

GetMetricWithLabelValues won't panic. WithLabelValues would only panic if you give it an incorrect number of parameters (e.g. your metric declares 2 labels but you only passed 1 value).
In general, it's better to let WithLabelValues crash the program since it means that there's a programmatic error.

@simonpasquier you are right, WithLabelValues would only panic. Misread the code 👍

jhadvig · 2021-10-13T09:05:02Z

why are other RC (201+) OK ?

Seems redundant for GET responses. Removed httpOK and explicitly check 200.

jhadvig · 2021-10-13T09:07:40Z

Here Im not sure if this should be part of the middleware code. Since this serves only the purpose of the metrics endpoint. I think this should be part of the handler itself (maybe as a wrapper?)

Moved this part to a new metricsHandler in server.go to preprocess the request and leave middleware.go unchanged.

zonggen · 2021-10-13T15:36:16Z

Thank you for the reviews @jhadvig!
Updated and squashed commits. Addressed most of them except the GetMetricWithLabelValues comment. PTAL.

jhadvig · 2021-10-13T22:06:30Z

@zonggen thanks for addressing the comments. Unfortunately the PR needs rebase due to #10224

Also I see that the backend CI test is failing:

simonpasquier · 2021-10-14T07:18:10Z

(nit) I would suggest to shorten the label names since from the metric names already tell that it relates to console and Helm.

Suggested change

consoleHelmChartNameLabel = "console_helm_chart_name"

consoleHelmChartVersionLabel = "console_helm_chart_version"

consoleHelmChartNameLabel = "chart_name"

consoleHelmChartVersionLabel = "chart_version"

Adds prometheus metrics dependencies for helm. Result of `go mod vendor` and `go mod tidy`. Signed-off-by: Allen Bai <abai@redhat.com>

Works with openshift/console-operator#601 to add a console_helm_install_count counter vector metric. This change will increment the counter each time a user installs a Helm chart in the console. Closes: https://issues.redhat.com/browse/HELM-235 Signed-off-by: Allen Bai <abai@redhat.com> helm/metrics: update according to code reviews Signed-off-by: Allen Bai <abai@redhat.com> helm/metrics: add metrics for helm release upgrades and uninstalls Signed-off-by: Allen Bai <abai@redhat.com>

zonggen · 2021-10-14T15:17:04Z

/test e2e-gcp-console

zonggen · 2021-10-15T13:15:29Z

/test e2e-gcp-console

jhadvig

/lgtm
/approve

openshift-ci · 2021-10-18T11:45:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dperaza4dustbit, jhadvig, zonggen

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

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

openshift-bot · 2021-10-18T11:53:32Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-10-18T12:05:35Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

jhadvig · 2021-10-19T06:09:49Z

/hold for approvals
/assign @yapei @opayne1 @RickJWagner

opayne1 · 2021-10-19T20:43:30Z

/label docs-approved

yapei · 2021-10-26T07:37:13Z

Install/Uninstall Helm Releases and check the metrics data

$ oc -n openshift-console exec console-76cf6658f8-w9ghl -- curl -k -H "Authorization: Bearer $token" 'https://xxxx:8443/metrics' | grep "helm"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7023    0  7023    0     0   403k      0 --:--:-- --:--:-- --:--:--  403k
# HELP console_helm_installs_total Number of Helm installations from console by chart name and version.
# TYPE console_helm_installs_total counter
console_helm_installs_total{chart_name="dotnet",chart_version="0.0.1"} 1
console_helm_installs_total{chart_name="infinispan",chart_version="0.2.0"} 1
console_helm_installs_total{chart_name="quarkus",chart_version="0.0.3"} 1
console_helm_installs_total{chart_name="vault",chart_version="0.17.0"} 1
# HELP console_helm_uninstalls_total Number of Helm release uninstallations from console by chart name and version.
# TYPE console_helm_uninstalls_total counter
console_helm_uninstalls_total{chart_name="infinispan",chart_version="0.2.0"} 1
console_helm_uninstalls_total{chart_name="vault",chart_version="0.17.0"} 1

zonggen · 2021-10-26T11:50:37Z

A friendly bump @RickJWagner @yapei, is there anything else needed from my side to get this approved? :)

yapei · 2021-10-27T01:47:08Z

tested Install/Uninstall/Upgrade of Helm releases, working as intended

# oc -n openshift-console exec console-67f47f4469-4nmqn -- curl -k -H "Authorization: Bearer $token" 'https://10.130.0.37:8443/metrics' | grep "helm"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7173    0  7173    0     0   437k      0 --:--:-- --:--:-- --:--:--  466k
# HELP console_helm_installs_total Number of Helm installations from console by chart name and version.
# TYPE console_helm_installs_total counter
console_helm_installs_total{chart_name="dotnet",chart_version="0.0.1"} 1
console_helm_installs_total{chart_name="jenkins",chart_version="0.0.3"} 1
console_helm_installs_total{chart_name="quarkus",chart_version="0.0.3"} 1
# HELP console_helm_uninstalls_total Number of Helm release uninstallations from console by chart name and version.
# TYPE console_helm_uninstalls_total counter
console_helm_uninstalls_total{chart_name="dotnet",chart_version="0.0.1"} 1
# HELP console_helm_upgrades_total Number of Helm release upgrades from console by chart name and version.
# TYPE console_helm_upgrades_total counter
console_helm_upgrades_total{chart_name="jenkins",chart_version="0.0.3"} 1
console_helm_upgrades_total{chart_name="quarkus",chart_version="0.0.3"} 1

/label qe-approved

RickJWagner · 2021-10-27T19:12:52Z

/label px-approved

zonggen · 2021-10-27T21:35:41Z

/unhold

zonggen · 2021-10-27T21:36:13Z

/retest

openshift-bot · 2021-10-27T21:58:03Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 6, 2021

openshift-ci Bot requested review from dperaza4dustbit and florkbr October 6, 2021 20:37

zonggen force-pushed the helm-metrics branch from 1806dba to 1775b0a Compare October 6, 2021 20:38

zonggen force-pushed the helm-metrics branch from 1775b0a to 14d1795 Compare October 7, 2021 15:41

zonggen changed the title ~~[WIP] metrics: add /metrics endpoint and console_helm_install_count metric~~ metrics: add /metrics endpoint and console_helm_install_count metric Oct 7, 2021

openshift-ci Bot added kind/dependency-change Categorizes issue or PR as related to changing dependencies and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 7, 2021

zonggen mentioned this pull request Oct 7, 2021

manifests: resources required for adding helm metrics in console openshift/console-operator#601

Merged

zonggen force-pushed the helm-metrics branch from eaf9157 to d9c22cc Compare October 7, 2021 18:34

simonpasquier reviewed Oct 8, 2021

View reviewed changes

dperaza4dustbit approved these changes Oct 12, 2021

View reviewed changes

openshift-ci Bot assigned dperaza4dustbit Oct 12, 2021

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 12, 2021

zonggen force-pushed the helm-metrics branch 3 times, most recently from 280cf92 to 1da9214 Compare October 12, 2021 20:47

jhadvig requested changes Oct 13, 2021

View reviewed changes

zonggen force-pushed the helm-metrics branch 2 times, most recently from 967308a to a9a8a42 Compare October 13, 2021 15:31

zonggen force-pushed the helm-metrics branch from a9a8a42 to 50835fc Compare October 13, 2021 15:48

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2021

simonpasquier reviewed Oct 14, 2021

View reviewed changes

vendor: dependencies for helm prom metrics

3552496

Adds prometheus metrics dependencies for helm. Result of `go mod vendor` and `go mod tidy`. Signed-off-by: Allen Bai <abai@redhat.com>

zonggen force-pushed the helm-metrics branch from 9305b1d to e4bf21e Compare October 14, 2021 13:54

jhadvig approved these changes Oct 18, 2021

View reviewed changes

openshift-ci Bot assigned jhadvig Oct 18, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2021

openshift-ci Bot assigned opayne1 Oct 19, 2021

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2021

openshift-ci Bot assigned RickJWagner and yapei Oct 19, 2021

openshift-ci Bot added the docs-approved Signifies that Docs has signed off on this PR label Oct 19, 2021

openshift-ci Bot added the qe-approved Signifies that QE has signed off on this PR label Oct 27, 2021

openshift-ci Bot added the px-approved Signifies that Product Support has signed off on this PR label Oct 27, 2021

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2021

openshift-merge-robot merged commit 5087780 into openshift:master Oct 28, 2021

spadgett added this to the v4.10 milestone Dec 8, 2021

	Help: "Number of Helm installation from console.",
	Help: "Number of Helm installations from console broken down by chart and version.",

-	consoleHelmChartNameLabel    = "console_helm_chart_name"
-	consoleHelmChartVersionLabel = "console_helm_chart_version"
+	consoleHelmChartNameLabel    = "chart_name"
+	consoleHelmChartVersionLabel = "chart_version"

Conversation

zonggen commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kyoto commented Oct 7, 2021

Uh oh!

zonggen commented Oct 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zonggen commented Oct 8, 2021

Uh oh!

dperaza4dustbit left a comment

Choose a reason for hiding this comment

Uh oh!

jhadvig 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!

zonggen Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

zonggen Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zonggen commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhadvig commented Oct 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zonggen commented Oct 14, 2021

Uh oh!

zonggen commented Oct 15, 2021

Uh oh!

jhadvig left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Oct 18, 2021

Uh oh!

openshift-bot commented Oct 18, 2021

Uh oh!

zonggen commented Oct 6, 2021 •

edited

Loading

zonggen Oct 13, 2021 •

edited

Loading

zonggen Oct 13, 2021 •

edited

Loading

zonggen commented Oct 13, 2021 •

edited

Loading