Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reworks the prometheus metrics to adhere to best practices #5687

Merged

Conversation

wozniakjan
Copy link
Member

@wozniakjan wozniakjan commented Apr 12, 2024

This is continuing work from #5174. I have rebased on main, resolved conflicts and working on fixing the e2e bug as described in #5174 (comment).

Original text from @wonko

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?)

There are two parts to this PR

Checklist

Fixes #4854
Closes #5174
Relates to kedacore/keda-docs#1374

@wozniakjan wozniakjan requested a review from a team as a code owner April 12, 2024 08:46
@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

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

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

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

@wozniakjan wozniakjan force-pushed the feature/rework-prometheus-metric-names branch from 0d4fc28 to e9d6065 Compare April 12, 2024 11:05
@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

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

@wozniakjan wozniakjan force-pushed the feature/rework-prometheus-metric-names branch 2 times, most recently from 3697c27 to 30e16b9 Compare April 12, 2024 11:43
@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

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

@wozniakjan
Copy link
Member Author

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

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

@wonko
Copy link
Contributor

wonko commented Apr 12, 2024

thx for picking this up @wozniakjan

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 12, 2024

hmm, now the e2e test failure is odd

 75     opentelemetry_metrics_test.go:720:
 76             Error Trace:    /__w/keda/keda/tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go:720
 77                                         /__w/keda/keda/tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go:458
 78             Error:          Should be true
 79             Test:           TestOpenTelemetryMetrics
 80             Messages:       keda_internal_scale_loop_latency not available

but I can observe both keda_internal_scale_loop_latency as well as keda_internal_scale_loop_latency_seconds

$ curl localhost:8080/metrics 2>/dev/null | grep keda_internal_scale_loop_latency
# HELP keda_internal_scale_loop_latency DEPRECATED - will be removed in 2.16: use 'keda_internal_scale_loop_latency_seconds' instead.
# TYPE keda_internal_scale_loop_latency gauge
keda_internal_scale_loop_latency{namespace="default",resource="php-apache-deployment-2",type="scaledobject"} 1
# HELP keda_internal_scale_loop_latency_seconds Total deviation (in seconds) between the expected execution time and the actual execution time for the scaling loop.
# TYPE keda_internal_scale_loop_latency_seconds gauge
keda_internal_scale_loop_latency_seconds{namespace="default",resource="php-apache-deployment-2",type="scaledobject"} 0.001352704

can't even get this to reproduce locally, tests just pass on every run

IIMAGE_REGISTRY=docker.io MAGE_REPO=wozniakjan E2E_TEST_REGEX=sequential ENABLE_OPENTELEMETRY=true make publish deploy e2e-test-local

...
Executing tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go, attempt "one"
Execution of tests/sequential/opentelemetry_metrics/opentelemetry_metrics_test.go, attempt "one" has passed
...

@wozniakjan wozniakjan force-pushed the feature/rework-prometheus-metric-names branch 6 times, most recently from 6c0c04f to db27705 Compare April 15, 2024 14:54
…tency and keda_scaler_metrics_latency

Signed-off-by: Jan Wozniak <[email protected]>
@wozniakjan wozniakjan force-pushed the feature/rework-prometheus-metric-names branch from db27705 to 25783ee Compare April 15, 2024 15:07
@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 15, 2024

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

@wozniakjan
Copy link
Member Author

tests finally pass, ptal @kedacore/keda-core-contributors

@wozniakjan wozniakjan mentioned this pull request Apr 15, 2024
35 tasks
@tomkerkhove
Copy link
Member

@wozniakjan It looks like kedacore/keda-docs#1258 is not owned by you, but linked. Is this one still up to date or do we need another one?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: Jan Wozniak <[email protected]>
@wozniakjan
Copy link
Member Author

It looks like kedacore/keda-docs#1258 is not owned by you, but linked. Is this one still up to date or do we need another one?

correct, kedacore/keda-docs#1258 was implemented by @wonko just like the majority of this PR originates in @wonko's PR #5174

There are a couple of minor suggestions in kedacore/keda-docs#1258, perhaps I can open a new docs PR that reflects reviewer suggestions.

We'll need a new issue for this; if I understand correctly we are just renaming them and keep the old ones.

correct, the old ones remained, just in the description KEDA will inform users these are deprecated. The new ones should be in alignment with prometheus conventions.

@wozniakjan
Copy link
Member Author

wozniakjan commented Apr 16, 2024

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

@zroubalik
Copy link
Member

There are a couple of minor suggestions in kedacore/keda-docs#1258, perhaps I can open a new docs PR that reflects reviewer suggestions.

if these suggestions are the only additions we can merge them in the existing PR

@zroubalik
Copy link
Member

There are a couple of minor suggestions in kedacore/keda-docs#1258, perhaps I can open a new docs PR that reflects reviewer suggestions.

if these suggestions are the only additions we can merge them in the existing PR

oh, I can see the PR is opened against 2.13, we should move that to 2.14. @wozniakjan could you please open a new doc PR then?

@wozniakjan
Copy link
Member Author

hey @zroubalik, with the docs PR kedacore/keda-docs#1374 approved, is there anything else I should do here or is this PR good to merge?

@zroubalik zroubalik enabled auto-merge (squash) April 19, 2024 17:07
@zroubalik zroubalik disabled auto-merge April 19, 2024 17:08
@zroubalik zroubalik merged commit 3722269 into kedacore:main Apr 19, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standardize prometheus naming convention for keda metrics
4 participants