Add helm prometheus metrics support and handle MultiNamespace scenario for metrics #2603

camilamacedo86 · 2020-02-27T01:46:24Z

Description of the change:

Add support for Metrics with MultiNamespace scenario: the kubemetrics are exports from the operator namespace by default, however, if be the scenario where the WATCH_NAMESPACE has a list of namespaces then all will be used to export the metrics
Add Prometheus metrics support to Helm-based operators. (Create the ServiceMonitor as should be)
Changed the default scaffold to export metrics in order to allow this and fix issues found as clean the code by centralizing the logic used
Keep the Ansible, Helm and GO with the same logic to export metrics

Motivation for the change:

Avoid users to perform inappropriate solutions and or ave wrong conclusions regards to it. See: Metrics of the Cluster scoped operator causes can't list unstructured object errors #1858 (comment)
Add support for Metrics with MultiNamespace scenario (E.g OLM Integration - the values from targetNamespace will be added to the WATCH_NAMESPACES)
Fix helm since it was not creating the service Monitor and keep all types with the same behaviours.
Increase the Maintenace ability

camilamacedo86 · 2020-03-10T10:57:44Z

Hi @lilic,

Really tks for your review. All your suggestions are addressed, feel free to check it again.

jmrodri

/lgtm

fabianvf · 2020-03-10T14:14:50Z

internal/scaffold/cmd_test.go

 	// Generate and serve custom resource specific metrics.
 	err = kubemetrics.GenerateAndServeCRMetrics(cfg, ns, filteredGVK, metricsHost, operatorMetricsPort)
 	if err != nil {
 		return err
 	}
 	return nil
 }
+
+// getNamespacesForMetrics wil return all namespaces which will be used to export the metrics


I may be missing something, why is this function duplicated here?

The cmd_test.go needs to be == cmd.go. It will verify the scaffold impl.

fabianvf

Seems like a lot of the metrics code is duplicated, is there a way we can centralize the pieces of logic that are shared to make this easier to maintain in the future? If that sort of refactor is out of scope I'd still like to track it somewhere

fabianvf · 2020-03-10T14:16:51Z

pkg/ansible/run.go

@@ -285,3 +283,20 @@ func getAnsibleDebugLog() bool {
 	}
 	return val
 }
+
+// getNamespacesForMetrics wil return all namespaces which will be used to export the metrics


Is there a way we can centralize this logic? It seems like it's duplicated several times

Moved to the lib.

fabianvf · 2020-03-10T14:17:13Z

pkg/helm/run.go

 	}
 	return nil
 }
+
+// getNamespacesForMetrics wil return all namespaces which will be used to export the metrics
+func getNamespacesForMetrics(operatorNs string) ([]string, error) {


fourth time this is implemented

The same implementation to generate the metrics is used for Ansible, Helm and GO.
The other is the tests which are updated with the changes made.

fabianvf · 2020-03-10T14:17:56Z

test/test-framework/cmd/manager/main.go

 	// Generate and serve custom resource specific metrics.
 	err = kubemetrics.GenerateAndServeCRMetrics(cfg, ns, filteredGVK, metricsHost, operatorMetricsPort)
 	if err != nil {
 		return err
 	}
 	return nil
 }
+
+// getNamespacesForMetrics wil return all namespaces which will be used to export the metrics
+func getNamespacesForMetrics(operatorNs string) ([]string, error) {


can we move this to a library function and just call it repeatedly?

fabianvf · 2020-03-10T14:19:39Z

pkg/helm/run.go

+
+// serveCRMetrics gets the Operator/CustomResource GVKs and generates metrics based on those types.
+// It serves those metrics on "http://metricsHost:operatorMetricsPort".
+func serveCRMetrics(cfg *rest.Config, operatorNs string, gvks []schema.GroupVersionKind) error {


Does the implementation of this function vary by operator type or can we use a single implementation?

The implementation is quite similar for all types.
However, Ansible and Helm will pass the gvks when in the GO we will filter it.

fabianvf · 2020-03-10T15:24:04Z

pkg/kube-metrics/metrics.go

+
+	// Generate metrics from the WATCH_NAMESPACES value if it contains multiple namespaces
+	if strings.Contains(watchNamespace, ",") {
+		ns = strings.Split(watchNamespace, ",")


Do we want to always export metrics in the operatorNs or is it intentional that we leave it out if specific watchNamespaces are set?

The WATCH_NAMESPACES can be the operator namespace (namespaced-scope), empty in the case of cluster-scoped or the multi namespace scenario.

So, the idea here is:
By default export the metrics from the operator namespace unless it is the multi namespace which is the case that we know the List of namespaces that the operator will be dealing wth.

fabianvf

/lgtm

CHANGELOG.md

openshift-ci-robot · 2020-03-10T16:05:25Z

New changes are detected. LGTM label has been removed.

…lded files. #2625 - Add info over the changes required in the default scaffold done in the `main.go` file to address bug fixes and improvements made in metrics. Related to PR's #2606, #2603 and #2601

openshift-ci-robot requested review from jmrodri and joelanford February 27, 2020 01:46

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 27, 2020

camilamacedo86 changed the title ~~Improvements in the scaffolded serveCRMetrics~~ Metrics for MultiNamespace scenario Feb 27, 2020

camilamacedo86 requested review from estroz, lilic, hasbro17, varshaprasad96, asmacdo, fabianvf and jmccormick2001 and removed request for joelanford February 27, 2020 01:49

camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Feb 27, 2020

Metrics for MultiNamespace scenario operator-framework#2603

c34f6aa

camilamacedo86 force-pushed the improve-metrics-namespaces branch from bfe93ad to c34f6aa Compare February 27, 2020 01:53

camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Feb 27, 2020

Metrics for MultiNamespace scenario operator-framework#2603

082a4c2

camilamacedo86 force-pushed the improve-metrics-namespaces branch from c34f6aa to 082a4c2 Compare February 27, 2020 01:55

camilamacedo86 added the metrics label Feb 27, 2020

camilamacedo86 removed the request for review from lilic February 27, 2020 12:31

camilamacedo86 pushed a commit to camilamacedo86/operator-sdk that referenced this pull request Feb 27, 2020

Metrics for MultiNamespace scenario operator-framework#2603

ad4ce32

camilamacedo86 force-pushed the improve-metrics-namespaces branch from 082a4c2 to ad4ce32 Compare February 27, 2020 13:30

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2020

camilamacedo86 added kind/feature Categorizes issue or PR as related to a new feature. kind/bug Categorizes issue or PR as related to a bug. labels Feb 27, 2020

camilamacedo86 mentioned this pull request Feb 27, 2020

fix: creation of monitor service when operator is cluster-scoped #2601

Merged

camilamacedo86 added kind/bug Categorizes issue or PR as related to a bug. and removed kind/bug Categorizes issue or PR as related to a bug. labels Feb 28, 2020

camilamacedo86 mentioned this pull request Mar 5, 2020

Add to migration doc steps regards changes over metrics to the scaffolded files. #2625

Merged

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 6, 2020

camilamacedo86 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2020

camilamacedo86 changed the title ~~Fix Helm metrics implementation and add support for Metrics handle the MultiNamespace scenario~~ Add helm prometheus metrics support and handle MultiNamespace scenario for metrics Mar 10, 2020

immprove cangelog

1f37f1a

camilamacedo86 removed kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 10, 2020

jmrodri approved these changes Mar 10, 2020

View reviewed changes

openshift-ci-robot assigned jmrodri Mar 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2020

fabianvf reviewed Mar 10, 2020

View reviewed changes

camilamacedo86 requested a review from fabianvf March 10, 2020 14:18

fabianvf suggested changes Mar 10, 2020

View reviewed changes

jmccormick2001 approved these changes Mar 10, 2020

View reviewed changes

centralizing the func in the metrics lib

a5e8596

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2020

camilamacedo86 requested a review from fabianvf March 10, 2020 14:36

fix lint

70a59f8

fabianvf reviewed Mar 10, 2020

View reviewed changes

camilamacedo86 requested a review from fabianvf March 10, 2020 15:33

fabianvf approved these changes Mar 10, 2020

View reviewed changes

openshift-ci-robot assigned fabianvf Mar 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2020

camilamacedo86 commented Mar 10, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

6dbe9a6

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2020

camilamacedo86 merged commit 829bfa2 into operator-framework:master Mar 10, 2020

camilamacedo86 deleted the improve-metrics-namespaces branch March 10, 2020 16:38

vebken-splunk mentioned this pull request Apr 27, 2021

Upgrade Operator SDK to v0.18.2 splunk/splunk-operator#312

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add helm prometheus metrics support and handle MultiNamespace scenario for metrics #2603

Add helm prometheus metrics support and handle MultiNamespace scenario for metrics #2603

camilamacedo86 commented Feb 27, 2020 •

edited

Loading

camilamacedo86 commented Mar 10, 2020 •

edited

Loading

jmrodri left a comment

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020

fabianvf left a comment

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020 •

edited

Loading

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020

fabianvf Mar 10, 2020

camilamacedo86 Mar 10, 2020 •

edited

Loading

fabianvf left a comment

openshift-ci-robot commented Mar 10, 2020

Add helm prometheus metrics support and handle MultiNamespace scenario for metrics #2603

Add helm prometheus metrics support and handle MultiNamespace scenario for metrics #2603

Conversation

camilamacedo86 commented Feb 27, 2020 • edited Loading

camilamacedo86 commented Mar 10, 2020 • edited Loading

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianvf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 Mar 10, 2020 • edited Loading

Choose a reason for hiding this comment

fabianvf left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 10, 2020

camilamacedo86 commented Feb 27, 2020 •

edited

Loading

camilamacedo86 commented Mar 10, 2020 •

edited

Loading

camilamacedo86 Mar 10, 2020 •

edited

Loading

camilamacedo86 Mar 10, 2020 •

edited

Loading