feat: Provide Prometheus metrics for total number of custom resources. #3719

v-shenoy · 2022-10-02T19:09:00Z

Docs PR - kedacore/keda-docs#954

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated and is aligned with our changelog requirements

Relates to #2637, #2638, #2639

v-shenoy · 2022-10-02T19:10:13Z

I'll update the tests and the docs after I also send PRs for #2637, #2638, if it's alright?
@JorTurFer @tomkerkhove

controllers/keda/clustertriggerauthentication_controller.go

tomkerkhove · 2022-10-03T05:40:42Z

Fine for me. Changed PR description not to close the issue

v-shenoy · 2022-10-03T11:30:17Z

I'll update the tests and the docs after I also send PRs for #2637, #2638, if it's alright? @JorTurFer @tomkerkhove

I'll be sending that PR after this one has been merged as it'll build on some changes I made here.

v-shenoy · 2022-10-05T23:07:12Z

@JorTurFer @tomkerkhove Any thoughts on this PR? I need this to be merged before working on the others as it will build on the changes here. Don't want to duplicate them across PRs.

JorTurFer · 2022-10-06T15:02:23Z

Sorry, I'm a bit busy atm, I'll take a look today or max tomorrow :(

JorTurFer

Looking good
Can we add this new metric to the metrics e2e test?

controllers/keda/clustertriggerauthentication_controller.go

v-shenoy · 2022-10-07T11:23:40Z

Looking good Can we add this new metric to the metrics e2e test?

I was planning on updating tests after adding prometheus metrics for all CRDs.

v-shenoy · 2022-10-07T11:26:12Z

Yeah, I think a finalizer would be a "cleaner" solution. Also helps in futureproofing, if more complex metrics / features are to be added.

v-shenoy · 2022-10-07T12:28:30Z

Actually, never mind. I'll just try to do all crd metrics and e2e test in this PR itself considering they're very coupled. @JorTurFer

JorTurFer · 2022-10-07T13:07:42Z

Actually, never mind. I'll just try to do all crd metrics and e2e test in this PR itself considering they're very coupled. @JorTurFer

No worries, you can add the e2e test in another PR with all the metrics

JorTurFer · 2022-10-07T13:09:01Z

My only concern is about the finalizer, maybe we should add it due to is clearer, atm we are already using them in other resources. WDYT @zroubalik ? Is this an overkill?

v-shenoy · 2022-10-07T22:02:19Z

My only concern is about the finalizer, maybe we should add it due to is clearer, atm we are already using them in other resources. WDYT @zroubalik ? Is this an overkill?

I also agree that the finalizers are cleaner than calling the method under the error block.

v-shenoy · 2022-10-07T22:03:11Z

Added metrics for all custom resources - SOs, SJs, TAs, CTAs, and updated the e2e test as well.

v-shenoy · 2022-10-07T23:42:28Z

Docs PR - kedacore/keda-docs#954

JorTurFer

Looking good
Could you move new finalizers to split files (like in case of so/sj). It's the code it's clearer 😄

v-shenoy · 2022-10-08T03:36:40Z

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

v-shenoy · 2022-10-08T03:37:53Z

Looking good Could you move new finalizers to split files (like in case of so/sj). It's the code it's clearer 😄

Moved them to separate files.

Had to pull out code into common functions because the linter was complaining about duplicate code. Struggled a bit with the Go type system, before declaring an interface that both reconcilers satisfy to avoid having ugly switch cases in the common code.

JorTurFer

LGTM!
Awesome job ❤️
Due to there are changes in the controllers part, let's wait till @zroubalik takes a look too

tomkerkhove

See kedacore/keda-docs#954 (comment) but basically I think it's a lot more user-friendly if we have 4 metrics:

keda_operator_scaledobject_totals - Total number of KEDA ScaledObjects per namespace resources.
keda_operator_scaledjobs_totals - Total number of KEDA ScaledJobs per namespace resources.
keda_operator_triggerauthentication_totals - Total number of KEDA TriggerAuthentication per namespace resources.
keda_operator_clustertriggerauthentication_totals - Total number of KEDA ClusterTriggerAuthentication resources.

tomkerkhove · 2022-10-24T06:09:20Z

See kedacore/keda-docs#954 (comment) but basically I think it's a lot more user-friendly if we have 4 metrics:

keda_operator_scaledobject_totals - Total number of KEDA ScaledObjects per namespace resources.

keda_operator_scaledjobs_totals - Total number of KEDA ScaledJobs per namespace resources.

keda_operator_triggerauthentication_totals - Total number of KEDA TriggerAuthentication per namespace resources.

keda_operator_clustertriggerauthentication_totals - Total number of KEDA ClusterTriggerAuthentication resources.

@JorTurFer @zroubalik can you please share your thoughts on this so that we can make the changes that are required (if need be)?

JorTurFer · 2022-10-24T07:47:38Z

See kedacore/keda-docs#954 (comment) but basically I think it's a lot more user-friendly if we have 4 metrics:

keda_operator_scaledobject_totals - Total number of KEDA ScaledObjects per namespace resources.

keda_operator_scaledjobs_totals - Total number of KEDA ScaledJobs per namespace resources.

keda_operator_triggerauthentication_totals - Total number of KEDA TriggerAuthentication per namespace resources.

keda_operator_clustertriggerauthentication_totals - Total number of KEDA ClusterTriggerAuthentication resources.

@JorTurFer @zroubalik can you please share your thoughts on this so that we can make the changes that are required (if need be)?

Let's discuss in the docs PR directly to have only one thread about this 😄

CHANGELOG.md

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

zroubalik

Looking good! just a small nit about using constants as suggested in CTA_controller.

controllers/keda/clustertriggerauthentication_controller.go

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

zroubalik · 2022-11-04T13:49:24Z

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

zroubalik

great feature!

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy · 2022-11-04T19:07:32Z

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

JorTurFer · 2022-11-05T02:30:12Z

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

v-shenoy · 2022-11-07T00:41:36Z

Can this be merged now? @JorTurFer

JorTurFer · 2022-11-07T10:11:00Z

yes, sorry, this was ready to be merged

v-shenoy requested a review from a team as a code owner October 2, 2022 19:09

v-shenoy commented Oct 2, 2022

View reviewed changes

controllers/keda/clustertriggerauthentication_controller.go Outdated Show resolved Hide resolved

JorTurFer reviewed Oct 7, 2022

View reviewed changes

controllers/keda/clustertriggerauthentication_controller.go Outdated Show resolved Hide resolved

v-shenoy force-pushed the crd-metrics branch from a9ad115 to dec8f51 Compare October 7, 2022 22:00

v-shenoy changed the title ~~feat: Provide Prometheus metrics for total numer of (Cluster)TriggerAuthentication objects~~ feat: Provide Prometheus metrics for total number of custom resources. Oct 7, 2022

v-shenoy mentioned this pull request Oct 7, 2022

Document CRD totals Prometheus metrics exposed by the operator kedacore/keda-docs#954

Merged

1 task

JorTurFer reviewed Oct 7, 2022

View reviewed changes

JorTurFer approved these changes Oct 9, 2022

View reviewed changes

tomkerkhove suggested changes Oct 10, 2022

View reviewed changes

v-shenoy force-pushed the crd-metrics branch from 138cc43 to 9d08028 Compare November 1, 2022 07:00

v-shenoy requested review from JorTurFer and tomkerkhove November 2, 2022 06:53

tomkerkhove reviewed Nov 2, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

v-shenoy added 11 commits November 4, 2022 16:18

Add Prometheus metrics for ClusterTriggerAuthentication totals

e8d8677

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Update CHANGELOG.

913ef5b

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Add finalizers for (Cluster)TriggerAuthentication custom resources

1cdb181

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Add Prometheus metrics for ScaledObject totals

c380f34

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Add Prometheus metrics for ScaledJob totals

f670825

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Add e2e test

309147c

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Update CHANGELOG

5639253

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Move finalization logic into separate files.

6839abd

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Change metric name

2c981d6

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Change metric name

96bbc4f

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

Change metric name

721dbd3

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

v-shenoy force-pushed the crd-metrics branch from 55f269d to 721dbd3 Compare November 4, 2022 10:48

zroubalik reviewed Nov 4, 2022

View reviewed changes

controllers/keda/clustertriggerauthentication_controller.go Outdated Show resolved Hide resolved

Use constants

e4ff597

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tomkerkhove requested review from tomkerkhove and zroubalik November 4, 2022 13:23

zroubalik approved these changes Nov 4, 2022

View reviewed changes

Fix changelog 🤦

f3881f8

Signed-off-by: Vighnesh Shenoy <vshenoy@microsoft.com>

tomkerkhove approved these changes Nov 7, 2022

View reviewed changes

tomkerkhove merged commit bcc85ec into kedacore:main Nov 7, 2022

v-shenoy deleted the crd-metrics branch November 7, 2022 09:06

This was referenced Nov 7, 2022

Provide a metric for total amount of ScaledObjects #2637

Closed

Provide a metric for total amount of ScaledJobs #2638

Closed

Provide a metric for total amount of (Cluster)TriggerAuthentication #2639

Closed

zroubalik mentioned this pull request Nov 7, 2022

chore: move Prometheus related metrics to prommetrics package #3827

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Provide Prometheus metrics for total number of custom resources. #3719

feat: Provide Prometheus metrics for total number of custom resources. #3719

v-shenoy commented Oct 2, 2022 •

edited by tomkerkhove

Loading

v-shenoy commented Oct 2, 2022

tomkerkhove commented Oct 3, 2022 •

edited

Loading

v-shenoy commented Oct 3, 2022

v-shenoy commented Oct 5, 2022

JorTurFer commented Oct 6, 2022

JorTurFer left a comment

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022 •

edited

Loading

JorTurFer commented Oct 7, 2022

JorTurFer commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

JorTurFer left a comment

v-shenoy commented Oct 8, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Oct 8, 2022 •

edited

Loading

JorTurFer left a comment

tomkerkhove left a comment

tomkerkhove commented Oct 24, 2022

JorTurFer commented Oct 24, 2022

zroubalik left a comment

zroubalik commented Nov 4, 2022 •

edited by github-actions bot

Loading

zroubalik left a comment

v-shenoy commented Nov 4, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Nov 5, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Nov 7, 2022

JorTurFer commented Nov 7, 2022

feat: Provide Prometheus metrics for total number of custom resources. #3719

feat: Provide Prometheus metrics for total number of custom resources. #3719

Conversation

v-shenoy commented Oct 2, 2022 • edited by tomkerkhove Loading

Checklist

v-shenoy commented Oct 2, 2022

tomkerkhove commented Oct 3, 2022 • edited Loading

v-shenoy commented Oct 3, 2022

v-shenoy commented Oct 5, 2022

JorTurFer commented Oct 6, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022 • edited Loading

JorTurFer commented Oct 7, 2022

JorTurFer commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

v-shenoy commented Oct 7, 2022

JorTurFer left a comment

Choose a reason for hiding this comment

v-shenoy commented Oct 8, 2022 • edited by github-actions bot Loading

v-shenoy commented Oct 8, 2022 • edited Loading

JorTurFer left a comment

Choose a reason for hiding this comment

tomkerkhove left a comment

Choose a reason for hiding this comment

tomkerkhove commented Oct 24, 2022

JorTurFer commented Oct 24, 2022

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik commented Nov 4, 2022 • edited by github-actions bot Loading

zroubalik left a comment

Choose a reason for hiding this comment

v-shenoy commented Nov 4, 2022 • edited by github-actions bot Loading

JorTurFer commented Nov 5, 2022 • edited by github-actions bot Loading

v-shenoy commented Nov 7, 2022

JorTurFer commented Nov 7, 2022

v-shenoy commented Oct 2, 2022 •

edited by tomkerkhove

Loading

tomkerkhove commented Oct 3, 2022 •

edited

Loading

v-shenoy commented Oct 7, 2022 •

edited

Loading

v-shenoy commented Oct 8, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Oct 8, 2022 •

edited

Loading

zroubalik commented Nov 4, 2022 •

edited by github-actions bot

Loading

v-shenoy commented Nov 4, 2022 •

edited by github-actions bot

Loading

JorTurFer commented Nov 5, 2022 •

edited by github-actions bot

Loading