Add metrics port to operator by jihuin · Pull Request #216 · knative/serving-operator

jihuin · 2019-11-13T01:54:54Z

Proposed Changes

Added metrics port in operator.yaml so the operator can report metrics to 9090 port.
Changed the component name from serving-operator to serving_operator because - is not allowed in metric name for prometheus exporter.

Currently the operator only emits the metrics on reconcile count, reconcile latency and work queue depth, that are inherited from knative.dev/controller.

Release Note

NONE

… serving" This reverts commit 86d46bd.

knative-prow-robot

@jihuin: 0 warnings.

Details

In response to this:

Fixes #192

Proposed Changes

Added metrics port in operator.yaml so the operator can report metrics to 9090 port.

Changed the component name from serving-operator to operator because - is not allowed in metric name for prometheus exporter.

Currently the operator only emits the metrics on reconcile count, reconcile latency and work queue depth, that are inherited from knative.dev/controller.

Release Note
NONE

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.

jihuin · 2019-11-13T01:56:18Z

/assign @anniefu @trshafer

jihuin · 2019-11-13T02:00:55Z

/assign @garron

anniefu · 2019-11-13T02:17:55Z

/lgtm

houshengbo · 2019-11-13T16:48:26Z

cmd/manager/main.go

 		log.Fatal("Error building kubeconfig", err)
 	}
-	sharedmain.MainWithConfig(signals.NewContext(), "serving-operator", cfg, knativeserving.NewController)
+	sharedmain.MainWithConfig(signals.NewContext(), "operator", cfg, knativeserving.NewController)


Why do you change the name here?

Because the component name serving-operator will be passed to the prometheus exporter as a prefix in the metric name, but the symbol - is not allowed in the prometheus metric name. Meanwhile, single-word application/component name is recommended by prometheus and all other components in knative serving have single-word names.

Is serving.operator supported? I lean on to keep the full name.

Only letters, digits and underscores are supported, and single-word prefix is recommended. See https://prometheus.io/docs/practices/naming/

Since all knative serving components have single-word names, e.g. webhook, activator, autoscalar, it makes sense to align with the current naming pattern.

Good catch @jihuin I noticed the same, and I know that ALL other knative components NOW using _ underscore. For instance:

knative/eventing@36058f4

Since we have more than one operator, let's please use serving_operator !

We can use the underscore.

@jihuin can you sent a PR to use serving_operator with underscore ?

Changed to serving_operator

houshengbo · 2019-11-13T16:49:40Z

config/operator.yaml

+            - name: CONFIG_LOGGING_NAME
+              value: config-logging
+            - name: CONFIG_OBSERVABILITY_NAME
+              value: config-observability


I do not think we need to add CONFIG_LOGGING_NAME and CONFIG_OBSERVABILITY_NAME, anymore.
By default, knative/pkg will pick config-logging and config-observability.

It's true that knative/pkg will watch config-logging and config-observability configmaps by default if nothing is specified. Maybe it is meaningful to add here to let the users be aware of the purposes of these two configmaps.

Though the configmap filenames are quite self-explaining, the users can not see if or where the configmaps are watched unless digging into knative/pkg

knative/pkg@9491151
I just had this PR merged. If config-logging and config-observability are missing, the controller should be launched without problem. With this PR, would it be fine, if config-logging and config-observability are missing??

@jihuin again, you are correct here.

See knative/eventing@36058f4 for adding port (the CONFIG_XYZ_NAME were already in the yamls, eg:

https://github.com/knative/eventing/blob/master/config/channels/in-memory-channel/500-controller.yaml#L37-L40

If the configmaps are missing and not watched, the exporter won't be set up and no metric is exported, but no information is provided in the logs. The configs here can be informative.

houshengbo · 2019-11-13T17:00:44Z

@jihuin Is there any test we can leverage to check if metrics are exposed?
Is 9090 the only port designated to metrics or is it something we can configure?

jihuin · 2019-11-14T00:31:04Z

@houshengbo The metrics are reported through knative.dev/pkg/controller, and are tested in the same package: https://github.com/knative/pkg/blob/master/controller/stats_reporter_test.go

The port number is configurable, and 9090 is the default port number of Prometheus server.

houshengbo · 2019-11-14T00:40:23Z

@jihuin I would like to test this change locally, could you tell me the steps? Thx.

jihuin · 2019-11-14T01:11:01Z

@houshengbo Steps to test metrics with Prometheus exporter (default):

Forward the local port 9090 to the port 9090 on operator pod via the command
kubectl port-forward ${{operator_pod_name}} 9090
Check the metrics by visiting localhost:9090/metrics in the browser, or run curl localhost:9090/metrics in the terminal

matzew

let;s change the component name to serving_operator

matzew · 2019-11-14T14:49:55Z

@jihuin can u report on your CLA status ?

jihuin · 2019-11-14T18:42:48Z

@googlebot rescan

matzew · 2019-11-15T15:11:20Z

/lgtm
/approve

matzew · 2019-11-15T16:04:16Z

@n3wscott I think this now looks good... mind approving ?

I am not an approver here, on the serving operator 😿

houshengbo

/lgtm
/approve

knative-prow-robot · 2019-11-15T18:22:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, jihuin, matzew

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jihuin and others added 6 commits November 1, 2019 16:10

Added metrics port to operator yaml

ac3c5fa

Merge remote-tracking branch 'upstream/master'

1b8e709

Added stats reporter to emit metrics when reconciling knative serving

86d46bd

Revert "Added stats reporter to emit metrics when reconciling knative…

a75e3d4

… serving" This reverts commit 86d46bd.

Sync with upstream

885f020

Changed the component name from 'serving_operator' to 'operator'

b8e0c3a

knative-prow-robot reviewed Nov 13, 2019

View reviewed changes

knative-prow-robot requested review from houshengbo and jcrossley3 November 13, 2019 01:55

knative-prow-robot added the size/XS label Nov 13, 2019

knative-prow-robot assigned anniefu and trshafer Nov 13, 2019

knative-prow-robot assigned garron Nov 13, 2019

knative-prow-robot added the lgtm label Nov 13, 2019

houshengbo reviewed Nov 13, 2019

View reviewed changes

matzew suggested changes Nov 14, 2019

View reviewed changes

googlebot added the cla: yes Author(s) signed a CLA. label Nov 14, 2019

Changed the component name to serving_operator

568f225

knative-prow-robot removed the lgtm label Nov 14, 2019

knative-prow-robot assigned matzew Nov 15, 2019

knative-prow-robot added the lgtm label Nov 15, 2019

houshengbo approved these changes Nov 15, 2019

View reviewed changes

knative-prow-robot assigned houshengbo Nov 15, 2019

knative-prow-robot added the approved label Nov 15, 2019

knative-prow-robot merged commit 6d87a77 into knative:master Nov 15, 2019

jihuin deleted the metrics-port branch November 19, 2019 05:02

Conversation

jihuin commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

jihuin commented Nov 13, 2019

Uh oh!

jihuin commented Nov 13, 2019

Uh oh!

anniefu commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houshengbo commented Nov 13, 2019

Uh oh!

jihuin commented Nov 14, 2019

Uh oh!

houshengbo commented Nov 14, 2019

Uh oh!

jihuin commented Nov 14, 2019

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

matzew commented Nov 14, 2019

Uh oh!

jihuin commented Nov 14, 2019

Uh oh!

matzew commented Nov 15, 2019

Uh oh!

matzew commented Nov 15, 2019

Uh oh!

houshengbo left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Nov 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jihuin commented Nov 13, 2019 •

edited

Loading