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

Cleanup metric names inside scalers #2260

Merged
merged 9 commits into from
Nov 10, 2021
Merged

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Nov 8, 2021

After #2123, we can ensure that metric names are unique inside ScaledObject. Before it, we used several mechanism inside scalers to avoid conflicts in metric names (like adding part of masked connectionString) those are not needed right now.
IMHO, we should review all scalers and remove those mechanisms. This will clear the code, unify how we are calculating metric names, and reduce risks related with exposing secrets

These listed metrics are the expected result for each scaler:

  • Artemis: sx-artemis-{queueName}
  • Apache Kafka: sx-kafka-{topic}
  • AWS Cloudwatch: sx-aws-cloudwatch-{dimensionName}
  • AWS Kinesis: sx-aws-kinesis-{streamName}
  • AWS SQS: sx-aws-sqs-{queueName}
  • Azure Blob: sx-azure-blob-{blobContainerName | customUserName}
  • Azure EventHub: sx-azure-eventhub-{eventhubConnection}
  • Azure Log Analytics: sx-azure-log-analytics-{workspaceId | customUserName} (No changes)
  • Azure Monitor: sx-azure-monitor-{azureMonitorName}
  • Azure Pipelines: sx-azure-pipelines-{poolId}
  • Azure Service Bus: sx-azure-servicebus-{queueName | topicName}
  • Azure Storage Queue: sx-azure-queue-{queueName}
  • Cassandra: sx-cassandra-{keyspace | customUserName} (No changes)
  • Cron: sx-cron-{timezone}-{start}-{end} (No changes)
  • External: sx-{metricName} (No changes)
  • Google Cloud Platform‎ Pub/Sub: sx-gcp-ps-{subscripotionName} (No changes)
  • Graphite: sx-graphite-{metricName} (No changes)
  • Huawei Couldeye: sx-huawei-cloudeye-{metricName}
  • IBM MQ: sx-ibmmq-{queueName}
  • InfluxDB: sx-influxdb-{organizationName | userCustomName}
  • Kubernetes Workload: sx-workload-{namespace}
  • Liiklus Topic: sx-liiklus-{topic}
  • Metrics API: sx-metric-api-{valueLocation}
  • MongoDB: sx-mongodb-{collection | userCustomName}
  • MSSQL: sx-mssql-{database | host | customUserName}
  • MySQL: sx-mysql-{dbName} (No changes)
  • NAT Streaming: sx-stan-{subject}
  • OpenStack Metric: sx-openstack-metric-{metricId}
  • OpenStack Swift: sx-openstack-swift-{metricName}
  • PostgreSQL: sx-postgresql{ |-userCustomName}
  • Prometheus: sx-prometheus-{metricName}
  • RabbitMQ: sz-rabbitmq-{queueName | userCustomName}
  • Redis Lists: sx-redis-{listName} (No changes)
  • Redis Lists Cluster: sx-redis-{listName} (No changes)
  • Redis Streams: sx-redis-streams-{streamName}
  • Redis Streams Cluster: sx-redis-streams-{streamName}
  • Selenium Grid Scaler: sx-seleniumgrid-{browserName}
  • Solace PubSub: sx-solace-{queueName}-{metricType}

Checklist

Fixes #2191

Jorge Turrado and others added 3 commits November 8, 2021 20:01
Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: jorturfer <[email protected]>
Signed-off-by: jorturfer <[email protected]>
@JorTurFer
Copy link
Member Author

/run-e2e

Signed-off-by: jorturfer <[email protected]>
Signed-off-by: jorturfer <[email protected]>
@JorTurFer
Copy link
Member Author

Could you take a look and share your thoughts @zroubalik ?
If you agree with the cleanup, I will open this as PR and modify the docs

@JorTurFer
Copy link
Member Author

InfluxDB e2e test has a timeout during the startup, but it works.
Maybe we could increase the timeout from 180 sec to (IDK) 300

Signed-off-by: jorturfer <[email protected]>
@JorTurFer
Copy link
Member Author

/run-e2e solace.test.ts

@JorTurFer
Copy link
Member Author

/run-e2e

@zroubalik
Copy link
Member

zroubalik commented Nov 9, 2021

@JorTurFer looking great!

I like the unification. I'd do a few minor changes:

  • Metrics API: sx-http-{valueLocation} -> sx-metric-api-{valueLocation} So it is not confused with the HTTP Add On or any other potential HTTP based scaler.
  • Google Cloud Platform‎ Pub/Sub: sx-gcp-{subscripotionName} -> sx-gcp-ps-{subscriptionName} To mark that it is GCP Pub/Sub one.

@JorTurFer JorTurFer marked this pull request as ready for review November 9, 2021 12:02
@JorTurFer
Copy link
Member Author

@zroubalik PTAL
I applied the feedback and I added a test to validate GetMetricSpecForScaling in metrics-api because we didn't have it yet.
Related with e2e tests, I have tried several times and except solacer (which failed because it was wrong, and it's already fixed), the others fails randomly

@zroubalik
Copy link
Member

zroubalik commented Nov 9, 2021

@JorTurFer the following job has been canceled, because of an action on this PR: https://github.com/kedacore/keda/actions/runs/1439326562

@JorTurFer JorTurFer changed the title WIP - Cleanup metric names inside scalers Cleanup metric names inside scalers Nov 9, 2021
@zroubalik zroubalik merged commit 2446b8e into kedacore:main Nov 10, 2021
@JorTurFer JorTurFer deleted the clean_metrics branch November 11, 2021 15:55
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.

Review and cleanup metric names inside scalers
2 participants