Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop references to prometheus monitors for Metrics Server #492

Closed
wants to merge 1 commit into from

Conversation

rajatvig
Copy link

@rajatvig rajatvig commented Jun 30, 2023

Checklist

  • I have verified that my change is according to the deprecations & breaking changes policy
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • README is updated with new configuration values (if applicable) learn more
  • A PR is opened to update KEDA core (repo) (if applicable, ie. when deployment manifests are modified)

Fixes

Removes monitoring code snippets for Keda Metrics Server

Signed-off-by: Rajat Vig <rvig@etsy.com>
@rajatvig rajatvig requested a review from a team as a code owner June 30, 2023 13:03
@JorTurFer
Copy link
Member

JorTurFer commented Jul 3, 2023

Hi!
I think that we shouldn't remove these references because metrics server still exposes Prometheus metrics: https://keda.sh/docs/2.11/operate/prometheus/#metrics-server

Currently, it only exposes operator-sdk metrics, but maybe in the future we add more metrics again to the metrics server to improving the observability there.

WDYT @tomkerkhove @zroubalik ? Should we remove the references now and add them again when we expose more metrics throught metrics server?

@rajatvig
Copy link
Author

rajatvig commented Jul 3, 2023

@JorTurFer Had checked on the Metrics Server process. Nothing is bound to port 8080 on the process and as such there are no metrics.

@JorTurFer
Copy link
Member

@JorTurFer Had checked on the Metrics Server process. Nothing is bound to port 8080 on the process and as such there are no metrics.

We should have an error in metrics server because the operator-sdk should expose them, maybe we have some mistake there, let me check it

@JorTurFer
Copy link
Member

I can confirm that it's due to a bug. During this commit I removed the startup of the metrics server.
Definitively, I need to go deeper to know how to solve it, but the metrics server should expose the REST API metrics, so I'd not remove it.
@zroubalik @tomkerkhove ?

@rajatvig rajatvig closed this Jul 6, 2023
@rajatvig rajatvig deleted the RemoveMetricServerMonitor branch July 6, 2023 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants