-
Notifications
You must be signed in to change notification settings - Fork 221
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
metrics: move prometheusrules under metricServer #316
base: main
Are you sure you want to change the base?
Conversation
21cd7cf
to
fe9d272
Compare
Signed-off-by: Erik Nobel <[email protected]>
fe9d272
to
b2dc235
Compare
@tomkerkhove decided to just tag you on this since you also reviewed my last PR. I would love to hear your opinion on this; thanks! |
@@ -298,3 +286,15 @@ prometheus: | |||
# expr: sum by ( scaledObject , scaler) (rate(keda_metrics_adapter_scaler_errors[2m])) > 0 | |||
# for: 2m | |||
# labels: | |||
operator: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dont think you wanted to use operator
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is taht this change will break a lot of end-users. We'll need to be backwards compatible here (even if it's confusing).
Side question - @zroubalik Are we planning on exposing Prometheus metrics on the operator?
I do recognize that this is a breaking change, you want to support both for now? |
I think it's best to do, yes |
We actually already have metrics on the operator so we should make sure both are still accessible in a backwards compatible way - kedacore/keda#3586 So instead of removing the operator pieces, I'd just keep them in addition to your new additions. @JorTurFer as FYI |
I think having metrics on both pods is the way, as it makes sense to record certain metrics on particular pods. So, adding configuration for metric server while keeping the operator would be great. |
What is the status on this PR? This might be a bit outdated due to our recent Prometheus changes |
Hi, folks!
I don't mean to be pedantic, but the
keda-operator-metrics-apiserver
app is exposing these (https://keda.sh/docs/2.8/operate/prometheus/) metrics.At the moment we create Prometheus rules under the
prometheus.operator
directive which is subsequently used inkeda/templates/15-keda-prometheusrules.yaml
. The name of the Prometheus rule is also{{ .Values.operator.name }}
.This all is a bit confusing to me and frankly, doesn't make sense, since it belongs to the
metricServer
section.Would like to her you guys opinion on this :)
Checklist
Fixes #