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

feat: Add support custom metricName in RabbitMQ scaler #1976

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

rtnpro
Copy link
Contributor

@rtnpro rtnpro commented Jul 22, 2021

This change adds support for parsing custom metricName in RabbitMQ scaler, and help to workaround use cases where there's reconcile error for a ScaledObject having duplicate auto generated metricNames, e.g., multiple trigger on the same queue, mode but from different RabbitMQ hosts.

Checklist

Fixes #1975

@rtnpro
Copy link
Contributor Author

rtnpro commented Jul 22, 2021

I need some suggestions on how to fix the tests here: https://github.com/kedacore/keda/blob/main/pkg/scalers/rabbitmq_scaler_test.go#L94-L104. I see that there's no assertion on the generated Metadata, but checks to just assert if parsing trigger metadata was successful.

Should I add a new function and some test data to assert if we are parsing custom metricName properly?

@zroubalik
Copy link
Member

I need some suggestions on how to fix the tests here: https://github.com/kedacore/keda/blob/main/pkg/scalers/rabbitmq_scaler_test.go#L94-L104. I see that there's no assertion on the generated Metadata, but checks to just assert if parsing trigger metadata was successful.

Should I add a new function and some test data to assert if we are parsing custom metricName properly?

Yeah, that would be nice!

@zroubalik
Copy link
Member

@rtnpro would you mind fixing the conflicts, failures and linter problems? Thanks!

@rtnpro rtnpro force-pushed the scaler-rabbitmq-support-metricName branch 3 times, most recently from dea79f6 to 2e15f18 Compare August 2, 2021 07:42
@rtnpro
Copy link
Contributor Author

rtnpro commented Aug 2, 2021

@zroubalik I have updated the test data rabbitMQMetricIdentifiers to assert if custom metric names are used to generate metric names correctly. RabbitMQ scaler tests run successfully.

Let me know what you think about: 2e15f18

@tomkerkhove tomkerkhove changed the title Add suport custom metricName in RabbitMQ scaler feat: Add support custom metricName in RabbitMQ scaler Aug 2, 2021
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good, could you please update Changelog and update documentation on kedacore/keda-docs ? Thanks

Fixes kedacore#1975

Signed-off-by: Ratnadeep Debnath <rtnpro@gmail.com>
@rtnpro rtnpro force-pushed the scaler-rabbitmq-support-metricName branch from 2e15f18 to 415288d Compare August 2, 2021 15:32
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for the contribution!

@rtnpro
Copy link
Contributor Author

rtnpro commented Aug 2, 2021

@zroubalik I have added the docs fix here: kedacore/keda-docs#499. Thanks for reviewing the PR :)

@zroubalik zroubalik added this to the v2.4.0 milestone Aug 3, 2021
@zroubalik zroubalik self-assigned this Aug 3, 2021
@zroubalik zroubalik merged commit b20d57e into kedacore:main Aug 3, 2021
TsuyoshiUshio pushed a commit that referenced this pull request Aug 6, 2021
Signed-off-by: Ratnadeep Debnath <rtnpro@gmail.com>
Signed-off-by: Tsuyoshi Ushio <ushio@simplearchitect.com>
nilayasiktoprak pushed a commit to nilayasiktoprak/keda that referenced this pull request Oct 23, 2021
Signed-off-by: Ratnadeep Debnath <rtnpro@gmail.com>
Signed-off-by: nilayasiktoprak <nilayasiktoprak@gmail.com>
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.

RabbitMQ scaler does not support custom metricName
2 participants