search

Kuadrant / multicluster-gateway-controller

multi-cluster gateway controller, manages multi-cluster gateways based on gateway api and policy attachment
Apache License 2.0
11 stars 23 forks source link

Fix metrics service port #654

Closed averevki closed 11 months ago

averevki commented 12 months ago

Fix mapping for metrics service port: https://github.com/Kuadrant/multicluster-gateway-controller/blob/aa223edcfff267b738e63f0ac4798725a1bfeeff/config/default/manager_metrics_patch.yaml#L19-L21

openshift-ci[bot] commented 12 months ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: averevki Once this PR has been reviewed and has the lgtm label, please assign philbrookes for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/Kuadrant/multicluster-gateway-controller/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
david-martin commented 11 months ago

@averevki This change might conflict with, or already be solved by the Service/ServiceMonitor in https://github.com/Kuadrant/multicluster-gateway-controller/tree/main/config/prometheus

Those resources are used by https://github.com/Kuadrant/multicluster-gateway-controller/blob/main/hack/quickstart-metrics.sh in this metrics walkthrough

What problem are you seeing that led to you needing the change in this PR?

averevki commented 11 months ago

@averevki This change might conflict with, or already be solved by the Service/ServiceMonitor in https://github.com/Kuadrant/multicluster-gateway-controller/tree/main/config/prometheus

Those resources are used by https://github.com/Kuadrant/multicluster-gateway-controller/blob/main/hack/quickstart-metrics.sh in this metrics walkthrough

What problem are you seeing that led to you needing the change in this PR?

There was a problem where mgc-controller-manager was exposing the port under metrics name, but mgc-controller-manager-metrics-service service was targeting the port under the name https, so no metrics appeared. I see it as a simple port name mistake

It looks like everything is pointing to the metrics port in the Prometheus config that you sent. So I suppose it is only in mgc-controller-manager-metrics-service.

It is possible that mgc-controller-manager-metrics-service is not present anymore and the issue is already irrelevant. But if it is and Prometheus is supposed to be used with metrics port, do you know the intended use-case for this metrics service? Thanks

david-martin commented 11 months ago

do you know the intended use-case for this metrics service

It looks like it should have been removed. See comment thread https://github.com/Kuadrant/multicluster-gateway-controller/pull/389#discussion_r1284163440 and the (probably incorrectly closed automatically, but I now reopened) issue https://github.com/Kuadrant/multicluster-gateway-controller/issues/411

averevki commented 11 months ago

It looks like it should have been removed. See comment thread #389 (comment) and the (probably incorrectly closed automatically, but I now reopened) issue #411

I see... Then this PR is irrelevant and feel free to close it. Thank you for the explanation :+1: