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

metricName should be deprecated for Cassandra Scaler #4227

Closed
JorTurFer opened this issue Feb 12, 2023 · 5 comments
Closed

metricName should be deprecated for Cassandra Scaler #4227

JorTurFer opened this issue Feb 12, 2023 · 5 comments
Labels
good first issue Good for newcomers help wanted Looking for support from community

Comments

@JorTurFer
Copy link
Member

JorTurFer commented Feb 12, 2023

Proposal

Currently, Cassandra scaler requires a metricName which is used only for KEDA internal things (it's not sent to the prom server).
This requirement isn't necessary since we introduced mechanisms to ensure the unique metrics names inside the ScaledObject and we should make it optional and also deprecate it in favor of self-generated internal name

https://github.com/kedacore/keda/blob/main/pkg/scalers/cassandra_scaler.go#L151-L155

@yardenshoham
Copy link
Contributor

I'd like to give it a shot. It'll be my first time to KEDA. I've read CONTRIBUTING.md. Could you list some high-level TODO list here? What I did so far is adding logging to Cassandra's scaler metadata parsing to print a deprecation message, but I'm not sure I'm on the right track.

--- a/pkg/scalers/cassandra_scaler.go
+++ b/pkg/scalers/cassandra_scaler.go
@@ -48,7 +48,7 @@ func NewCassandraScaler(config *ScalerConfig) (Scaler, error) {

        logger := InitializeLogger(config, "cassandra_scaler")

-       meta, err := parseCassandraMetadata(config)
+       meta, err := parseCassandraMetadata(config, logger)
        if err != nil {
                return nil, fmt.Errorf("error parsing cassandra metadata: %w", err)
        }
@@ -67,7 +67,7 @@ func NewCassandraScaler(config *ScalerConfig) (Scaler, error) {
 }

 // parseCassandraMetadata parses the metadata and returns a CassandraMetadata or an error if the ScalerConfig is invalid.
-func parseCassandraMetadata(config *ScalerConfig) (*CassandraMetadata, error) {
+func parseCassandraMetadata(config *ScalerConfig, logger logr.Logger) (*CassandraMetadata, error) {
        meta := CassandraMetadata{}

        if val, ok := config.TriggerMetadata["query"]; ok {
@@ -150,6 +150,7 @@ func parseCassandraMetadata(config *ScalerConfig) (*CassandraMetadata, error) {

        if val, ok := config.TriggerMetadata["metricName"]; ok {
                meta.metricName = kedautil.NormalizeString(fmt.Sprintf("cassandra-%s", val))
+               logger.Info("metricName is deprecated and shouldn't be explicitly set in scaler trigger metadata")
        } else {
                meta.metricName = kedautil.NormalizeString(fmt.Sprintf("cassandra-%s", meta.keyspace))
        }

@JorTurFer
Copy link
Member Author

Yeah, that draft looks nice. The idea is to announce to end-users that the feature will be removed in v2.12 so maybe I'd include that info in the log, but that's all
😊😊

@yardenshoham
Copy link
Contributor

Do I need to make any changes to CHANGELOG.md or the KEDA docs?

@JorTurFer
Copy link
Member Author

I think that we can refer the main issue directly in the changelog to reduce the noise:
#4220

But yes, the first one who fixes one of the sub issues should update the changelog xD

@zroubalik
Copy link
Member

will be handled by #4220

@github-project-automation github-project-automation bot moved this from To Triage to Ready To Ship in Roadmap - KEDA Core Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Looking for support from community
Projects
Archived in project
Development

No branches or pull requests

3 participants