fix error to create service metrics when run operator-sdk up local

[camilamacedo86]

Description of the change:
Skip service metrics when is running out o the cluster

Motivation for the change:

Closes: #1702

LOCAL TEST

[joelanford]

I'm confused about the motivation for this. As far as I can tell, we already skip metrics service creation during up local

operator-sdk up local output from an operator built from master:

...
{"level":"info","ts":1573667590.2471445,"logger":"cmd","msg":"Could not generate and serve custom resource metrics","error":"operator run mode forced to local"}
{"level":"info","ts":1573667590.448851,"logger":"cmd","msg":"Could not create metrics Service","error":"failed to initialize service object for metrics: OPERATOR_NAME must be set"}
{"level":"info","ts":1573667590.6498501,"logger":"cmd","msg":"Could not create ServiceMonitor object","error":"no ServiceMonitor registered with the API"}
{"level":"info","ts":1573667590.6498818,"logger":"cmd","msg":"Install prometheus-operator in your cluster to create ServiceMonitor objects","error":"no ServiceMonitor registered with the API"}
...

Is this just about improving the log messages?

[camilamacedo86]

Hi @joelanford,

We do not skip actually. We just throw the error which makes the users thinking that something is wrong so the idea here is to keep as it is done for the leader and make clear that it was not created and why instead of throw errors. See:

{"level":"info","ts":1573666348.370976,"logger":"leader","msg":"Skipping leader election; not running in a cluster."}
`

[camilamacedo86]

HMO: Technically is a very simple change. It is a nit as you can see :-) But very helpful to not cause misunderstanding and improve the user experience. I mean, since the info does not clearly show a bug in the user point of view.

[openshift-ci-robot]

@camilamacedo86: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/marker	74934d8	link	/test marker
ci/prow/e2e-aws-ansible	74934d8	link	/test e2e-aws-ansible

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[JAORMX]

thanks! I was a bit confused by this error message 😄

[estroz]

/lgtm

[joelanford]

It looks like addMetrics() is a small wrapper around serveCRMetrics() that does some extra logging and error handling, right?

IMO, another function in the main.go template for this is a bit much, and I think we could add the extra error handling in line in the body of the main function.

If I understand correctly, it seems like this PR could be reduced to:

        if err = serveCRMetrics(cfg); err != nil {
-               log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
+               if errors.Is(err, k8sutil.ErrRunLocal) {
+                       log.Info("Skipping CR metrics server creation; not running in a cluster.")
+               } else {
+                       log.Info("Could not generate and serve custom resource metrics", "error", err.Error())
+               }
        }

[camilamacedo86]

The logs/msgs came from more than one point in the original code (See serveCRMetrics, CreateMetricsService, CreateServiceMonitors). The idea was centralized/encapsulate the logic to add the metrics and do its improvement.

{"level":"info","ts":1563485392.3758454,"logger":"cmd","msg":"Could not generate and serve custom resource metrics","error":"operator run mode forced to local"}
{"level":"info","ts":1563485392.4013908,"logger":"cmd","msg":"Could not create metrics Service","error":"failed to initialize service object for metrics: OPERATOR_NAME must be set"}
{"level":"info","ts":1563485392.4066937,"logger":"cmd","msg":"Could not create ServiceMonitor object","error":"no ServiceMonitor registered with the API"}

In this way, just the above suggestion still not attending the expectations.

[joelanford]

Ah I see now. The diff was confusing me :)

[joelanford]

We already skip metrics creation when running locally, right? This PR is about logging a more informative message to help the user understand that skipping metrics creation is not an error, right?

[camilamacedo86]

The current msgs/loggings give an impression that it is an error when is not.
I improved the description and move to changed section in the changelog.

[joelanford]

/lgtm