Use metricName value to set metric name #3182

vivekjainx86 · 2022-06-16T10:22:44Z

Signed-off-by: vivekjainx86 vivekjainx86@gmail.com

It fixes the issue #3180

Checklist

Commits are signed with Developer Certificate of Origin (DCO - learn more)
Tests have been added
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Changelog has been updated and is aligned with our changelog requirements

Fixes #3180

Relates to #

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

JorTurFer

Looking good,
Only small nit inline
Could you open a PR in keda-docs repo explaining this new parameter?

JorTurFer · 2022-06-16T10:36:16Z

pkg/scalers/predictkube_scaler.go

+	} else {
+		return nil, fmt.Errorf("no metricName given")
+	}


I'd not do this mandatory. I understand the reason to set it, but there are cases where this is not needed and we should use the default value (the current value).

Sure, will do that. Also, changes in keda-docs, you mean in this file? if not, can you please point me out?

Could you help me understand why do we need to introduce this parameter please? We are traing to get rid of it in general and rely on generating, as it is internal stuff.

In a scenario where PredictKube scaler is used in multiple ScaledObjects, there metric will be same, i.e. s0-predictkube-predictkube_metric.
The need of that parameter was to use user provided metricName value to uniquely identify the metric, similar to as used in Prometheus scaler. The current code doesn't do that.

OK and are you talking about uniqueness from KEDA side or from PredictKube? Becase on KEDA side the uniqueness is assured by the combination of metricName and label on ScaledObject. So having the same metric name for multiple SOs is not a problem, we are distingush them by labels.

Do you face any specific issue in your setup?

I am talking about uniqueness from PredictKube.
Currently, we have multiple ScaledObjects with multiple triggers (PredictKube scaler along with Prometheus scalers).
But in future, we might have multiple PredictKube scalers in single ScaledObject, then it will be difficult to identify the metric by using its index, and it will not be readable.

And for what exactly do you need to read that? That's pure internal thing.

And it's just a name it is not a metric (value) itself.

I understand it. The thing is we are plotting the metrics graph to analyse the triggers and it's related scaling pattern. For Prometheus scalers, we can see given metricName, but for PredictKube, we cannot. You can refer to attached screenshot.

Even if you say that it's internal thing, but it is visible in Keda metric, and can be used to plot graphs.

Can you just add ScaledObject to the graph? So you can distinguish the values?

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

Signed-off-by: Vivek Jain <38214848+vivekjainx86@users.noreply.github.com>

vivekjainx86 · 2022-06-20T10:22:56Z

Hello @JorTurFer , can you please re-review the PR? I have made suggested changes, also created PR in keda-docs. Please let me know if anything missing.

JorTurFer · 2022-06-20T15:38:55Z

Hello @JorTurFer , can you please re-review the PR? I have made suggested changes, also created PR in keda-docs. Please let me know if anything missing.

let's try till this is solved. TBH, I don't like so match giving this option because we changed it to make it internal and avoid user configs here

avdhoot · 2022-06-20T16:20:08Z

Hello @JorTurFer , can you please re-review the PR? I have made suggested changes, also created PR in keda-docs. Please let me know if anything missing.

let's try till this is solved. TBH, I don't like so match giving this option because we changed it to make it internal and avoid user configs here

We are talking about single scale object having multiple metrics. how to distinguish between them in graph & metrics?

zroubalik · 2022-06-21T13:05:05Z

Hello @JorTurFer , can you please re-review the PR? I have made suggested changes, also created PR in keda-docs. Please let me know if anything missing.

let's try till this is solved. TBH, I don't like so match giving this option because we changed it to make it internal and avoid user configs here

We are talking about single scale object having multiple metrics. how to distinguish between them in graph & metrics?

Each metric in ScaledObject has different name, so you can distinguish between them?

avdhoot · 2022-06-21T16:53:31Z

Please let us know which metric is distinguishable & more importantly readable/informative? Giving random metric name in keda_metrics_adapter_scaler_metrics_value metric will loose it is usefulness. cc @zroubalik

Metrics without fix

keda_metrics_adapter_scaler_metrics_value{metric="s0-predictkube-predictkube_metric", namespace="keda", pod="keda-metrics-apiserver-8bf54d668-ftcqr", scaledObject="xxx-web", scaler="PredictKubeScaler", scalerIndex="0"}
keda_metrics_adapter_scaler_metrics_value{metric="s1-predictkube-predictkube_metric", namespace="keda", pod="keda-metrics-apiserver-8bf54d668-ftcqr", scaledObject="xxx-web", scaler="PredictKubeScaler", scalerIndex="1"}

Metrics with fix

keda_metrics_adapter_scaler_metrics_value{metric="s0-predictkube-web_predicted_requests_per_second_per_pod", namespace="keda", pod="keda-metrics-apiserver-69d4794db4-p7n8t", scaledObject="xxx-web", scaler="PredictKubeScaler", scalerIndex="0"}
keda_metrics_adapter_scaler_metrics_value{metric="s1-predictkube-celery_predicted_rabbitmq_queue_messages_published_rate", namespace="keda", pod="keda-metrics-apiserver-69d4794db4-p7n8t", scaledObject="xxx-web", scaler="PredictKubeScaler", scalerIndex="1"}

Signed-off-by: Vivek Jain <38214848+vivekjainx86@users.noreply.github.com>

zroubalik · 2022-12-08T15:06:46Z

implemented in other way

avdhoot · 2022-12-08T15:25:21Z

@zroubalik can you please share PR link or commit link of new implementation for reference?

zroubalik · 2022-12-08T17:23:18Z

@avdhoot #3588

vivekjainx86 added 2 commits June 16, 2022 15:48

use metricName value to set metric name

765a1fe

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

remove prefix variable

199d1e1

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

vivekjainx86 requested a review from a team as a code owner June 16, 2022 10:22

JorTurFer reviewed Jun 16, 2022

View reviewed changes

vivekjainx86 added 2 commits June 16, 2022 16:07

fix variable errors

72c42f1

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

assign default value for metricName

3bc7f3f

Signed-off-by: vivekjainx86 <vivekjainx86@gmail.com>

vivekjainx86 mentioned this pull request Jun 17, 2022

Add metricName field in PredictKube kedacore/keda-docs#788

Open

1 task

vivekjainx86 requested a review from JorTurFer June 20, 2022 06:26

Merge branch 'main' into fix-issue-3180

4733a10

Signed-off-by: Vivek Jain <38214848+vivekjainx86@users.noreply.github.com>

v-shenoy mentioned this pull request Aug 24, 2022

Enhance azure data explorer scaler metric name with trigger.metadata.metricName #3589

Closed

Merge branch 'main' into fix-issue-3180

89f3b5b

Signed-off-by: Vivek Jain <38214848+vivekjainx86@users.noreply.github.com>

zroubalik closed this Dec 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use metricName value to set metric name #3182

Use metricName value to set metric name #3182

vivekjainx86 commented Jun 16, 2022 •

edited

Loading

JorTurFer left a comment •

edited

Loading

JorTurFer Jun 16, 2022

vivekjainx86 Jun 16, 2022

zroubalik Jun 16, 2022

vivekjainx86 Jun 16, 2022

zroubalik Jun 16, 2022 •

edited

Loading

vivekjainx86 Jun 16, 2022

zroubalik Jun 16, 2022

zroubalik Jun 16, 2022

vivekjainx86 Jun 16, 2022

zroubalik Jun 20, 2022

vivekjainx86 commented Jun 20, 2022

JorTurFer commented Jun 20, 2022

avdhoot commented Jun 20, 2022 •

edited

Loading

zroubalik commented Jun 21, 2022

avdhoot commented Jun 21, 2022

zroubalik commented Dec 8, 2022

avdhoot commented Dec 8, 2022 •

edited

Loading

zroubalik commented Dec 8, 2022

Use metricName value to set metric name #3182

Use metricName value to set metric name #3182

Conversation

vivekjainx86 commented Jun 16, 2022 • edited Loading

Checklist

JorTurFer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vivekjainx86 commented Jun 20, 2022

JorTurFer commented Jun 20, 2022

avdhoot commented Jun 20, 2022 • edited Loading

zroubalik commented Jun 21, 2022

avdhoot commented Jun 21, 2022

zroubalik commented Dec 8, 2022

avdhoot commented Dec 8, 2022 • edited Loading

zroubalik commented Dec 8, 2022

vivekjainx86 commented Jun 16, 2022 •

edited

Loading

JorTurFer left a comment •

edited

Loading

zroubalik Jun 16, 2022 •

edited

Loading

avdhoot commented Jun 20, 2022 •

edited

Loading

avdhoot commented Dec 8, 2022 •

edited

Loading