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

Add MongoDB scaler #1467

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Add MongoDB scaler #1467

merged 2 commits into from
Jan 15, 2021

Conversation

NUCsimple
Copy link
Contributor

Signed-off-by: carson [email protected]

This PR adds a new scaler for the mongo.

Checklist

Related to #1465.

@tomkerkhove
Copy link
Member

Can you fix DCO & our static checks please?

Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should call it mongo or mongodb?

CHANGELOG.md Outdated
@@ -19,6 +19,7 @@
### New
- Can use Pod Identity with Azure Event Hub scaler ([#994](https://github.com/kedacore/keda/issues/994))
- Introducing InfluxDB scaler ([#1239](https://github.com/kedacore/keda/issues/1239))
- Introducing Mongo scaler ([#1465](https://github.com/kedacore/keda/issues/1465))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Introducing Mongo scaler ([#1465](https://github.com/kedacore/keda/issues/1465))
- Introducing MongoDB scaler ([#1465](https://github.com/kedacore/keda/issues/1465))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it's better to call it mongodb scaler.

Copy link
Member

Choose a reason for hiding this comment

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

We should keep that naming consistent, in the go code all the references to the scaler are as mongo not mongodb. Shouldn't we change that as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, i will change them to mongoDB,at the same time,add e2e tests for this scaler.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

@NUCsimple NUCsimple force-pushed the add_mongo_scaler branch 2 times, most recently from 143197b to f17a965 Compare January 1, 2021 15:27
@NUCsimple
Copy link
Contributor Author

Can you fix DCO & our static checks please?

DCO is fixed,but static checks failed again, bacause of the cache.

@ringtail
Copy link

ringtail commented Jan 4, 2021

@tomkerkhove Could we trigger the pre-commit re-test.

@zroubalik
Copy link
Member

@NUCsimple thanks for the contribution! I'll get to the proper review later. Could you please add e2e tests for this scaler?

pkg/scalers/mongo.go Outdated Show resolved Hide resolved
@zroubalik zroubalik changed the title add mongo scaler Add MongoDB scaler Jan 4, 2021
@NUCsimple NUCsimple force-pushed the add_mongo_scaler branch 3 times, most recently from dc989e6 to 974ed74 Compare January 10, 2021 14:32
@NUCsimple
Copy link
Contributor Author

Hi, @zroubalik .Coud you please check it again? I have changed all the non-standard contents mentioned above.

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.

Minor formating related nits and mongoDB -> mongodb for consistency

@@ -0,0 +1,286 @@
package scalers
Copy link
Member

Choose a reason for hiding this comment

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

Could you please name this file mongodb_scaler.go so it is consistent with other scalers in the package.

defaultQueryValue = 1
)

var mongoDBLog = logf.Log.WithName("mongoDB_scaler")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var mongoDBLog = logf.Log.WithName("mongoDB_scaler")
var mongoDBLog = logf.Log.WithName("mongodb_scaler")

@@ -0,0 +1,87 @@
package scalers
Copy link
Member

Choose a reason for hiding this comment

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

Could you please name this file mongodb_scaler_test.go so it is consistent with other tests in the package.

@@ -474,6 +474,8 @@ func buildScaler(triggerType string, config *scalers.ScalerConfig) (scalers.Scal
return scalers.NewCPUMemoryScaler(corev1.ResourceMemory, config)
case "metrics-api":
return scalers.NewMetricsAPIScaler(config)
case "mongoDB":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case "mongoDB":
case "mongodb":

I'd make this lowercase, to be consistent with other scalers

func (s *mongoDBScaler) GetMetrics(ctx context.Context, metricName string, metricSelector labels.Selector) ([]external_metrics.ExternalMetricValue, error) {
num, err := s.getQueryResult()
if err != nil {
return []external_metrics.ExternalMetricValue{}, fmt.Errorf("failed to inspect momgo,because of %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return []external_metrics.ExternalMetricValue{}, fmt.Errorf("failed to inspect momgo,because of %v", err)
return []external_metrics.ExternalMetricValue{}, fmt.Errorf("failed to inspect mongoDB, because of %v", err)

}

if err = client.Ping(ctx, readpref.Primary()); err != nil {
return nil, fmt.Errorf("failed to ping mongoDB,because of %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("failed to ping mongoDB,because of %v", err)
return nil, fmt.Errorf("failed to ping mongoDB, because of %v", err)

if val, ok := config.TriggerMetadata["queryValue"]; ok {
queryValue, err := strconv.Atoi(val)
if err != nil {
return nil, "", fmt.Errorf("failed to convert %v to int,because of %v", queryValue, err.Error())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return nil, "", fmt.Errorf("failed to convert %v to int,because of %v", queryValue, err.Error())
return nil, "", fmt.Errorf("failed to convert %v to int, because of %v", queryValue, err.Error())

if s.client != nil {
err := s.client.Disconnect(context.TODO())
if err != nil {
mongoDBLog.Error(err, fmt.Sprintf("failed to close mongoDB connection,because of %v", err))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mongoDBLog.Error(err, fmt.Sprintf("failed to close mongoDB connection,because of %v", err))
mongoDBLog.Error(err, fmt.Sprintf("failed to close mongoDB connection, because of %v", err))


filter, err := json2BsonDoc(s.metadata.query)
if err != nil {
mongoDBLog.Error(err, fmt.Sprintf("failed to convert query param to bson.Doc,because of %v", err))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mongoDBLog.Error(err, fmt.Sprintf("failed to convert query param to bson.Doc,because of %v", err))
mongoDBLog.Error(err, fmt.Sprintf("failed to convert query param to bson.Doc, because of %v", err))


docsNum, err := s.client.Database(s.metadata.dbName).Collection(s.metadata.collection).CountDocuments(ctx, filter)
if err != nil {
mongoDBLog.Error(err, fmt.Sprintf("failed to query %v in %v,because of %v", s.metadata.dbName, s.metadata.collection, err))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mongoDBLog.Error(err, fmt.Sprintf("failed to query %v in %v,because of %v", s.metadata.dbName, s.metadata.collection, err))
mongoDBLog.Error(err, fmt.Sprintf("failed to query %v in %v, because of %v", s.metadata.dbName, s.metadata.collection, err))

Signed-off-by: 高强 <[email protected]>
Signed-off-by: 高强 <[email protected]>
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 for the contribution!

@tomkerkhove tomkerkhove merged commit efbcbe5 into kedacore:main Jan 15, 2021
ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021
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.

4 participants