keda icon indicating copy to clipboard operation
keda copied to clipboard

Reworks the prometheus metrics to adhere to best practices

Open wonko opened this issue 2 years ago • 12 comments

https://github.com/kedacore/keda/issues/4854

I reworked the prometheus metric names to follow conventions: added units where needed, and appended total.

Also passed a time.Duration down to the metrics handlers, as the conversion to units needs to happen when construction the metrics.

No tests were present, none added.

No idea where to track deprecation actions (when and how will the deprecated metrics be removed?)

Checklist

Fixes #4854

Relates to https://github.com/kedacore/keda-docs/pull/1258

wonko avatar Nov 09 '23 16:11 wonko

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

Learn more about:

github-actions[bot] avatar Nov 09 '23 16:11 github-actions[bot]

updated the e2e tests, didn't add anything for testing for the deprecation notice, seems silly to me to rework.

also made the needed changes in the webhook, as I only spotted those when going through the e2e tests.

not able to run the e2e tests locally, fingers crossed this passes

wonko avatar Nov 22 '23 15:11 wonko

tests fail, but I don't think it's the part I was fiddling with. Also, the ending newline in the json - no idea what's the idea there, my VSCode removes the newline, and a vim-edit seems to add one, but then it's one too many ...

wonko avatar Nov 22 '23 15:11 wonko

/run-e2e sequential Update: You can check the progress here

JorTurFer avatar Nov 22 '23 16:11 JorTurFer

fixed the DCO and the newline - seems to be all green on those. E2E is out of my league ;-)

wonko avatar Nov 23 '23 10:11 wonko

hey @wonko, would you be available to address the merge conflicts? I can see these files are now unable to merge.

        both modified:   CHANGELOG.md
        both modified:   pkg/metricscollector/metricscollectors.go
        both modified:   pkg/metricscollector/opentelemetry.go
        both modified:   pkg/metricscollector/prommetrics.go
        both modified:   pkg/scaling/scale_handler.go
        both modified:   tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go
        both modified:   tests/sequential/prometheus_metrics/prometheus_metrics_test.go

wozniakjan avatar Jan 11 '24 14:01 wozniakjan

hey @wonko, do you think you will be able to find some time to resolve the conflicts? It's ok if not, I can help out to cherry-pick your work and follow up in a new PR.

wozniakjan avatar Jan 30 '24 11:01 wozniakjan

@wozniakjan Been busy with some other stuff lately, but I could pick this up either this week, or this weekend latest.

wonko avatar Jan 30 '24 13:01 wonko

no pressure @wonko, I'm available at your convenience. I just think this is really solid work and it would be great to get this merged in the upcoming months :)

wozniakjan avatar Jan 30 '24 14:01 wozniakjan

no pressure @wonko, I'm available at your convenience. I just think this is really solid work and it would be great to get this merged in the upcoming months :)

@wozniakjan I think it's good now ... please have a good look at it, as I did the merge with a lot of interruptions ...

wonko avatar Feb 05 '24 14:02 wonko

/run-e2e sequential Update: You can check the progress here

zroubalik avatar Feb 12 '24 22:02 zroubalik

hey @wonko, the e2e tests seem to fail with metrics registration error

2024/02/12 22:24:44 maxprocs: Updating GOMAXPROCS=1: determined from CPU quota
panic: a previously registered descriptor with the same fully-qualified name as Desc{fqName: "keda_scaler_errors_total", help: "The total number of errors
encountered for each scaler.", constLabels: {}, variableLabels: {namespace,metric,scaledObject,scaler,triggerIndex,type}} has different label names or a different
help string

goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0x172ccd8?, {0xc000084e80?, 0x1, 0xc000c9f5d0?})
    /workspace/vendor/github.com/prometheus/client_golang/prometheus/registry.go:405 +0x78
github.com/kedacore/keda/v2/pkg/metricscollector.NewPromMetrics()
    /workspace/pkg/metricscollector/prommetrics.go:233 +0x30e
github.com/kedacore/keda/v2/pkg/metricscollector.NewMetricsCollectors(0xd?, 0x1)
    /workspace/pkg/metricscollector/metricscollectors.go:79 +0x25
main.main()
    /workspace/cmd/operator/main.go:148 +0xb3f

can you please take a look at it? The error location could mean it's related to the changes.

wozniakjan avatar Feb 21 '24 09:02 wozniakjan

Sure thing, I can investigate tomorrow.

wozniakjan avatar Apr 10 '24 17:04 wozniakjan