-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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(output): add mongodb output plugin #9923
Conversation
Thanks so much for the pull request! |
!signed-cla |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bustedware, thanks for this nice PR!
Please check if you can implement this plugin using the go.mongodb.org/mongo-driver
driver instead of using the unmaintained library used now. I have some more comments in the code...
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
… member variable. fmt statements used previouslly for logging/debugging are now using log instead
…e plugin interface. refactored some additional variable names and modified README.md to reflect those names.
…. using testutil for test metrics in mongodb_test output test.
- MongoDBGetCollections is now member function - removed TODO from Init() - ctx now using Background() from Connect() method
- no longer storing context in struct - removed errorneous comments and TODOs
…und MongoDBCreateTimeSeriesCollection
plugins/outputs/mongodb/mongodb.go
Outdated
CAFile string `toml:"cafile"` | ||
X509clientpem string `toml:"x509clientpem"` | ||
X509clientpempwd string `toml:"x509clientpempwd"` | ||
Allow_tls_insecure bool `toml:"allow_tls_insecure"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should add this option to TLSConfig
then for other plugins to profit as well. Do you think this can be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bustedware, we are getting closer. Some more comments in the code. Please try to export as less as possible. Looking forward to the next round!
// create time series collection when it doesn't exist | ||
myTestMetricName := "testMetricName" | ||
if !s.DoesCollectionExist(myTestMetricName) { | ||
err = s.MongoDBCreateTimeSeriesCollection(myTestMetricName) | ||
require.NoError(t, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be done by Write
automatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a different test, should be in its own func.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Write does this automatically. I have added a lot more tests. Please let me know if the tests span enough cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that MongoDBCreateTimeSeriesCollection is no longer exported and is now named createTimeSeriesCollection. I think leaving Write to perform the testing of this specific functionality should be enough.
…Dockerfile and mongodb.sh in mongodb output plugin
assign client only after checking err status. Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great improvements, 2 small comments!
plugins/outputs/mongodb/mongodb.go
Outdated
serverSelectionTimeoutSeconds := int64(s.ServerSelectTimeout / 1000000000) | ||
s.clientOptions = s.clientOptions.SetServerSelectionTimeout(time.Duration(serverSelectionTimeoutSeconds) * time.Second) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this conversion? s.ServerSelectTimeout
is already a Duration
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalid operation: mismatched types config.Duration and time.Duration - compiler MismatchedTypes. when I try:
s.clientOptions = s.clientOptions.SetServerSelectionTimeout(s.ServerSelectTimeout * time.Second)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check slack thread for more details
fixed misleading error message regarding a missing username from configuration Co-authored-by: Thomas Casteleyn <thomas.casteleyn@me.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bustedware you nicely worked this trough with @Hipska. Good job! I only have some minor comments, the most severe is the handling of the timeout which already is a duration.
Can you also elaborate on what you use that PKI (certificates) for?
error checking added to ensure name of collection returned is of type string to prevent panic. default value added for database option default value added for granularity option updated to use switch statement for metric granularity value checks using HasPrefix to be explicit instead of Contains() for Dsn checks updated to use err within local/in scope contexts of method calls removed superfilous lines from NoAuth test for brevity and cleanliness Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
…y now defaulted. removed return statement which will always be nil. golang fixed space formatting from previous commit suggestions
…ranularity setting. fixed mongodb.go to actually set the default values and not set the sample config. updated mongodb_test.go to pass new short test cases where config no longer requires database or granularity field to pass init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This leaves us with the timeout/duration setting. :-)
verified and applied suggestion for invoking SetServerSelectionTimeout Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
…Seconds variable which no longer exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks @bustedware for the nice PR and the constructive discussions!
…of config.Duration
🥳 This pull request decreases the Telegraf binary size by -1.09 % for linux amd64 (new size: 130.7 MB, nightly size 132.1 MB) 📦 Looks like new artifacts were built from this PR. Expand this list to get them here! 🐯Artifact URLs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Nice collaboration 👏
Added an output plugin to gather metrics and output them to mongodb v5.0+ time series collections