Kanister prometheus metrics: Controller Integration + ActionSet resolution counter exports #2247

mellon-collie · 2023-08-08T14:19:30Z

Change Overview

Following changes made under the kanister prometheus metrics helm feature flag:
-> This PR exports ActionSet resolution counter called kanister_action_set_resolutions_total, with "success" and "failure" label values - based on the result of the ActionSet.
-> This PR also integrates the Kanister-prometheus framework to the main Kanister controller interface. The integration involves the controller object taking in a prometheus registry and instantiating a metrics object that encapsulates all the Prometheus metric objects for the controller interface.

Pull request type

Please check the type of change your PR introduces:

Issues

fixes #issue-number

Test Plan

TextExecActionSet unit-test has been modified to ensure that "success" and "failure" counter increments are made successfully on the resolution of an ActionSet.
New unit-test called TestNilPrometheusRegistry has been added to test the behavior of the controller when a nil prometheus registry is passed and ensure that it does not panic.
Manual tests: Multiple action sets were created from the mysql blueprint example and the /metrics endpoint has been tested to see if the metrics are correctly exported.

Details:
When the a new Kanister operator pod is deployed, the kanister_action_set_resolutions_total metric is set to 0 in the /metrics endpoint:

Following the mysql-blueprints example from the examples directory, 2 ActionSets were created which perform mysql backups, both of which succeeded:

kanctl create actionset --action backup --namespace kanister --blueprint mysql-blueprint --statefulset mysql-test/mysql-release --profile mysql-test/<profile-name> --secrets mysql=mysql-test/mysql-release

Following this, the /metrics displays the following counter value:

💪 Manual
⚡ Unit test
💚 E2E

github-actions · 2023-08-08T14:19:44Z

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

viveksinghggits

LGTM
left some minor comments.

pkg/controller/controller.go

pkg/controller/metrics.go

pkg/controller/controller_test.go

pkg/kancontroller/kancontroller.go

pkg/controller/controller_test.go

viveksinghggits

LGTM

pkg/controller/controller_test.go

viveksinghggits · 2023-08-10T16:54:34Z

I have approved, we can wait for @pavannd1 to have a look as well.

…cs is enabled

go.mod

pavannd1 · 2023-08-18T22:23:55Z

pkg/controller/controller.go

@@ -435,17 +448,20 @@ func (c *Controller) runAction(ctx context.Context, t *tomb.Tomb, as *crv1alpha1
 	c.logAndSuccessEvent(ctx, fmt.Sprintf("Executing action %s", action.Name), "Started Action", as)
 	tp, err := param.New(ctx, c.clientset, c.dynClient, c.crClient, c.osClient, action)
 	if err != nil {
+		c.incrementActionSetResolutionCounterVec(ACTION_SET_COUNTER_VEC_LABEL_RES_FAILURE)


Could you remind me what the issue was with incrementing this in the caller of runAction i.e., handleActionSet inside the if block on line 421?

@pavannd1 There is no issue per se with respect to this PR. Considering that we do move the Inc() calls to line 421 now, and we decide to pair the resolution label dimension with another dimension, say action_type in future PRs, we would have to move back this Inc() from line 421 to the callee, because at line 421, we don't have the action_type information. Hence, I thought that it would be better to maintain the Inc() calls in the callee to avoid make an additional change when we add action_type dimension.

I think this would apply to any other label dimension we choose to use with the resolution dimension.

pkg/controller/controller_test.go

pavannd1 · 2023-08-18T22:35:53Z

pkg/controller/controller_test.go

@@ -630,6 +666,7 @@ func (s *ControllerSuite) TestExecActionSet(c *C) {
 			if !cancel {
 				err = s.waitOnActionSetState(c, as, final)
 				c.Assert(err, IsNil, Commentf("Failed case: %s", tc.name))
+				c.Assert(getCounterVecValue(s.ctrl.metrics.actionSetResolutionCounterVec, []string{tc.metricResolution}), Equals, oldValue+1, Commentf("Failed case: %s", tc.name))


What happens in the cancel case?

We're currently maintaining only success and failure resolutions as per our discussions. Hence cancel has essentially been ignored, and we will not be incrementing any values for the cancel case.

If we do decide to add cancel in the future, then we will have to add a new Inc() at the point where we figure out that the ActionSet has been cancelled.

It is a bit confusing because TestExecActionSet has been set up to "expect" a failure in the cancel case, but we aren't asserting anything for now. Let me know if it's better to pass in blank resolution status during the unit test.

pkg/controller/controller_test.go

pkg/controller/metrics.go

pavannd1

LGTM

infraq added this to In Progress in Kanister Aug 8, 2023

mellon-collie requested review from pavannd1 and viveksinghggits August 8, 2023 15:01

viveksinghggits reviewed Aug 9, 2023

View reviewed changes

mellon-collie force-pushed the kanister-prometheus-metrics-resolution branch 5 times, most recently from 9b3a423 to 2ac8dd8 Compare August 9, 2023 17:50

mellon-collie added the hold off merging label Aug 9, 2023

viveksinghggits reviewed Aug 10, 2023

View reviewed changes

pkg/controller/controller_test.go Outdated Show resolved Hide resolved

viveksinghggits reviewed Aug 10, 2023

View reviewed changes

pkg/controller/controller_test.go Outdated Show resolved Hide resolved

Base automatically changed from kanister-prometheus-skeleton-framework to master August 10, 2023 16:47

viveksinghggits approved these changes Aug 10, 2023

View reviewed changes

Kanister automation moved this from In Progress to Reviewer approved Aug 10, 2023

mellon-collie removed the hold off merging label Aug 10, 2023

pavannd1 added the hold off merging label Aug 11, 2023

mellon-collie added 10 commits August 14, 2023 12:38

Added metrics file in controller

161b789

Exporting resolution metrics for success and failure

c9c65f1

Adding unit test to ensure increments are done correctly

8e2489b

Added code to avoid panics if nil registry is passed

459c0f2

Added code to test for nil registry scenario

4be2959

Added code to read env variable which checks to see if kanister metri…

a4a10d1

…cs is enabled

moved synchronous failure increments to the callee

be80363

fixed issue with registry setting

7d1b5cf

renamed metric to include kanister_ prefix

01aeb30

Added godocs

1031809

mellon-collie added 4 commits August 14, 2023 12:38

Addressed Vivek's review comments

7828cbe

renamed dto to promtest

2eafd7e

renamed promtest to promgomodel

fd2ac02

Merge branch 'master' into kanister-prometheus-metrics-resolution

743d6ab

mellon-collie force-pushed the kanister-prometheus-metrics-resolution branch from 0d23528 to 743d6ab Compare August 14, 2023 16:54

mellon-collie added 2 commits August 14, 2023 12:57

fixed go.mod

a5097e1

Fixed testing apis

b7f5930

mellon-collie removed the hold off merging label Aug 14, 2023

pavannd1 reviewed Aug 18, 2023

View reviewed changes

mellon-collie force-pushed the kanister-prometheus-metrics-resolution branch from 0ee8f78 to f365d37 Compare August 18, 2023 23:16

Addressed Pavan's review comments

023af87

mellon-collie force-pushed the kanister-prometheus-metrics-resolution branch from f365d37 to 023af87 Compare August 18, 2023 23:17

mellon-collie mentioned this pull request Aug 19, 2023

Helper method to bound an ActionSet's "action type" to a fixed set for Prometheus Metrics #2256

Merged

10 tasks

pavannd1 approved these changes Aug 24, 2023

View reviewed changes

mellon-collie added the kueue label Aug 25, 2023

Merge branch 'master' into kanister-prometheus-metrics-resolution

3a22679

mergify bot merged commit 536ac9a into master Aug 25, 2023
14 checks passed

Kanister automation moved this from Reviewer approved to Done Aug 25, 2023

mergify bot deleted the kanister-prometheus-metrics-resolution branch August 25, 2023 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kanister prometheus metrics: Controller Integration + ActionSet resolution counter exports #2247

Kanister prometheus metrics: Controller Integration + ActionSet resolution counter exports #2247

mellon-collie commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023

viveksinghggits left a comment

viveksinghggits left a comment

viveksinghggits commented Aug 10, 2023

pavannd1 Aug 18, 2023

mellon-collie Aug 18, 2023

pavannd1 Aug 18, 2023

mellon-collie Aug 18, 2023

mellon-collie Aug 18, 2023

pavannd1 left a comment

Kanister prometheus metrics: Controller Integration + ActionSet resolution counter exports #2247

Kanister prometheus metrics: Controller Integration + ActionSet resolution counter exports #2247

Conversation

mellon-collie commented Aug 8, 2023 • edited Loading

Change Overview

Pull request type

Issues

Test Plan

github-actions bot commented Aug 8, 2023

viveksinghggits left a comment

Choose a reason for hiding this comment

viveksinghggits left a comment

Choose a reason for hiding this comment

viveksinghggits commented Aug 10, 2023

pavannd1 Aug 18, 2023

Choose a reason for hiding this comment

mellon-collie Aug 18, 2023

Choose a reason for hiding this comment

pavannd1 Aug 18, 2023

Choose a reason for hiding this comment

mellon-collie Aug 18, 2023

Choose a reason for hiding this comment

mellon-collie Aug 18, 2023

Choose a reason for hiding this comment

pavannd1 left a comment

Choose a reason for hiding this comment

mellon-collie commented Aug 8, 2023 •

edited

Loading