Remove metric reporting in KnativeServing.

[markusthoemmes]

Proposed Changes

These metrics don't seem very useful and it's doubtable anybody uses them.

/hold

Pending discussion if we want to ditch this.

Release Note

Removed metric reporting of KnativeServing changes.

/assign @houshengbo @aliok @jcrossley3

[knative-prow-robot]

@markusthoemmes: 0 warnings.

Details

In response to this:

Proposed Changes

These metrics don't seem very useful and it's doubtable anybody uses them.

/hold

Pending discussion if we want to ditch this.

Release Note

Removed metric reporting of KnativeServing changes.

/assign @houshengbo @aliok @jcrossley3

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.

[jcrossley3]

Looks like they were introduced here: knative/serving-operator#233

[houshengbo]

@trshafer @anniefu @garron @jihuin
What is the future plan for knative/serving-operator#233? How do we consume the metrics?

[markusthoemmes]

IMO the question is not "how" but "what do we expect them to tell us".

[anniefu]

I believe the goal with these metric are to give a sense of how much meaningful work the operator is actually doing, @jihuin might be able to add more clarification.

@houshengbo this uses the knative/pkg metrics pkg, so consuming metrics is the same as knative-serving, uses config-observability.yaml to determine metrics backend.

In general, Knative has a lot of instrumented metrics that I'm sure no one uses. I'm not sure if removing them is a good idea though. The fact that they were placed there means someone might be using them, and I don't think there's a policy for deprecating metrics right now.

We do have the metrics.backend-destination: none option in config-observability.yaml now to silence metrics. With the upcoming Knative metrics overhaul, I'm working on providing a spot to filter and drop metrics with the Otel Collector Service's filter processor.

[markusthoemmes]

@anniefu I'd argue that the metrics we have here are not a great indicator of how much meaningful work the operator is actually doing. I'm happy to take this into the WG for a broader general discussion though. I'd love to get a more clear view on why these have been added in the first place.

[jihuin]

The metrics were introduced in response to knative/serving-operator#192, which records that users configure the knative serving via installing/editing/deleting KnativeServing CR. It works for me to remove it if we agree this is not a good indicator of the work operator is doing.

[trshafer]

I don't personally have a strong opinion. They were originally made to emit similar reconcile metrics to the knative controller.

[houshengbo]

@markusthoemmes @anniefu @jihuin @trshafer
Let's say we remove this metric reporting for now, since it is not quite useful as we expected before.
If there is further need for this feature, we can reconsider the design.
Is everyone ok with it?

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes

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 [markusthoemmes]

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

[knative-prow-robot]

@markusthoemmes: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-sandbox-operator-upgrade-tests	c7c97d0	link	/test pull-knative-sandbox-operator-upgrade-tests

Full PR test history. Your PR dashboard.

Details

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.

[markusthoemmes]

Rebased and reran relevant scripts, this should be good to go if you still want to merge it @houshengbo

[houshengbo]

/lgtm

[houshengbo]

/hold cancel