feat(inputs.opensearch_query): Add opensearch_query input plugin #12393
Conversation
telegraf-tiger
bot
added
feat
There was a problem hiding this comment.
Thanks for putting this up! I have two high-level concerns:
- why does this need to be a second plugin?
- most of this is copy and paste from inputs.elasticsearch_query. I'd rather not have this much duplication. how much of this could be handled with a config variable or if not abstracted into the common folder?
I have done a very quick first pass through the PR as well, I realize some of these things already exist in the current plugin, but I'd like to clean up anything new that comes in.
|
|
||
| ## OpenSearch Support | ||
|
|
||
| This plugins is tested against OpenSearch 2.4.0. |
There was a problem hiding this comment.
Is it possible to test this against 1.3 to ensure some backwards compat before we accept this in?
There was a problem hiding this comment.
I can add testing for this. I wasn't worried about it since the 1.3.x line is currently maintenance-only, and will be EOL December 31, 2023.
There was a problem hiding this comment.
Thanks - I know someone is going to try and ask if it is supported so I'd rather find out now
There was a problem hiding this comment.
I tested it against 1.3.7, and it ran fine. I've add this container to the test suite.
I recently had a chat with CircleCI about this error. It is usually due to you having a CirlceCI account with an expired GitHub oauth. To resolve this, you need to log back into CircleCI with your usename/password if that is how you log in or if you use GitHub log, re-create your oauth/relogin with github. That should regenerate your token and then allow you to push a commit or close and reopen this PR and tests should run. I know the error message doesn't line up, and I have suggested them changing that, but that is what causes this error. And we do need tests to run :) |
As I'm sure you're aware, Elasticsearch and Opensearch are divergent products. The existing plugin supports old versions of ES, and (1) I didn't want to break workflows for existing users, and (2) I wanted there to be a clean spot to capture the delta.
The existing elasticsearch_query plugin doesn't support ES 7.10.2, which is what OpenSearch is based on. I used the olivere/elastic library for a quick win, since it already implements the structs for ES, but I would like to eventually move away from those, since the author has deprecated the library. This submission was intended to be an in-kind update for OS. I have more updates I want to put forward, such as support for multi-field aggregations that aren't implemented in the existing elasticsearch_query plugin. |
I appreciate this and I can understand how from your perspective this gives users what they need quickly. As a maintainer though, I now have to support two plugins that are nearly identical. I recognize the possible need for a second plugin, see my comment in #11345, as well as the need to change the underlying client library, see #12099. Ideally I would prefer us try to use the existing plugin and add an option for what version or interface we are connecting to as proposed in #12099 (this was for the elasticsearch input, but applies here as well). If that truly is not possible, then a new plugin using the supported library would be the next best option. |
|
Thanks for the links to the issues. I certainly understand the drive to not maintain two separate plugins, given their similarity. My thought on this, though, is that the similarity will diverge over time. Elastic already makes big API changes between major versions, and they've been especially hostile towards Opensearch, for obvious reasons. I'm not sure what the best course of action here is. Looking at the Would it make sense then to try and have two separate plugins? I know having an API selection option would be appealing, but given the ever increasing divergence, I think we'd eventually end up maintaining two plugins under one package anyway. |
Given your comments about the diverging of the two products it does sound like having two plugins does make sense. I would still want to see it using the new, supported library though. Thanks! |
… function; change metric name format
…; update README.md
|
Thanks for the feedback. I've left a couple questions in-line to hopefully tie up the loose ends on this PR. |
| err = o.Gather(&acc) | ||
| if (len(acc.Errors) > 0) && !tt.wantQueryResErr { | ||
| for _, err = range acc.Errors { | ||
| t.Errorf("OpensearchQuery.Gather() error: %v, wantQueryResErr %v", err, tt.wantQueryResErr) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| require.NoError(t, err) |
There was a problem hiding this comment.
Hmm what if we tt.wantQueryResErr but do not get any?
There was a problem hiding this comment.
Easy. You rewrite the test to properly check.
(len(acc.Errors) > 0) != tt.wantQueryResErr should fix that
…st all OpenSearch versions
There was a problem hiding this comment.
Looks good to me. Thanks for your work to make this fly @NullOranje!
|
@powersj there has been some significant changes, so please take a look again! |
Co-authored-by: Sven Rebhan <36194019+srebhan@users.noreply.github.com>
… handling cleanup
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_amd64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
Thank you for driving this! |
|
My pleasure. Looking forward to putting it to use! |
Required for all PRs
This plugin queries endpoints to obtain metrics from data stored in an OpenSearch cluster. The code is largely an update of the existing inputs.elasticsearch_query plugin.
The library in that plugin is incompatible with OpenSearch (and, in fact, all Elastic clusters >= 7.0.0). To resolve the issue, I made the following changes:
Use v7 of the high-level Elasticsearch API library olivere/elastic to process query requests and responses
Add the Amazon OpenSearch go client to support cluster queries
Future work should look at removing the deprecated olivere/elastic library.