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

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

Conversation

cdlliuy
Copy link
Contributor

@cdlliuy cdlliuy commented Aug 24, 2022

PR to address #3588

@cdlliuy cdlliuy requested a review from a team as a code owner August 24, 2022 08:32
…metricName

Signed-off-by: Ying Liu <lying@microsoft.com>
@cdlliuy cdlliuy force-pushed the ying/update_azure_data_explorer_metricname branch from 4a0e2ee to f1bca18 Compare August 24, 2022 08:35
@cdlliuy
Copy link
Contributor Author

cdlliuy commented Aug 24, 2022

add @v-shenoy for review

Copy link
Contributor

@v-shenoy v-shenoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the CHANGELOG with this change.

Could you also add a unit test checking the metric name is being used?

@@ -118,7 +118,13 @@ func parseAzureDataExplorerMetadata(config *ScalerConfig, logger logr.Logger) (*
}

// Generate metricName.
metadata.MetricName = GenerateMetricNameWithIndex(config.ScalerIndex, kedautil.NormalizeString(fmt.Sprintf("%s-%s", adxName, metadata.DatabaseName)))
metadataName, _ := getParameterFromConfig(config, "metricName", false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
metadataName, _ := getParameterFromConfig(config, "metricName", false)
metricName, err := getParameterFromConfig(config, "metricName", false)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@v-shenoy , updated in another approach. please help to review again.

@v-shenoy
Copy link
Contributor

Hmm, there has been some discussion regarding the need for this parameter - #3182

@JorTurFer @zroubalik can give their thoughts about this.

@JorTurFer
Copy link
Member

Let's discuss this in the issue, I wrote a comment there
#3588 (comment)

Signed-off-by: Ying Liu <lying@microsoft.com>
@cdlliuy
Copy link
Contributor Author

cdlliuy commented Sep 8, 2022

close this PR since the solution is changed

@cdlliuy cdlliuy closed this Sep 8, 2022
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.

3 participants