chore(inputs.elasticsearch_query): Migrate to official ES client#19288
chore(inputs.elasticsearch_query): Migrate to official ES client#19288alespour wants to merge 9 commits into
Conversation
| Addresses: cfg.urls, | ||
| Username: cfg.username, | ||
| Password: cfg.password, | ||
| Transport: roundTripper{client: cfg.httpClient}, |
There was a problem hiding this comment.
Why do we need this? Can't we simply do
| Transport: roundTripper{client: cfg.httpClient}, | |
| Transport: cfg.httpClient.Transport, |
There was a problem hiding this comment.
Unlike Transport, http.Client enforces timeout during the whole request-response lifecycle including response body-reading.
There was a problem hiding this comment.
I think so, without http.Client wrapper, stalled response during discovery could hang (discovery v6 is not cancellable via context). And that would block plugin shutdown via Stop().
But Client also handles cookies. Passing only Transport would lose it. We would have to handle cookies ourselves to cover cases when users enable auth cookie.
|
@alespour regarding the question if we need the v5 client if code works with v6, I'd say no. If you can cover multiple ES server versions with one client, go for it as long as this doesn't limit the queries or anything... |
|
@srebhan Thank you very much for very thorough review. I'm overhauling the PR. |
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
|
@srebhan PR is overhauled. Plugin uses concrete client implementation for each major version (v5, v6). Version-independent logic is now in shared functions (code blocks moved One thing that requires maintainers decision: v5 client does not support discovery. Now, when I can think of 3 solutions:
|
|
Thanks @alespour for the update, I will review in a minute... Regarding
Please issue a warning and silently ignore discovery for v5. I think this is best as it is unlikely someone is still on 5.x and uses discovery. Maybe also add a short comment in the |
| return "", 0, fmt.Errorf("parsing server version %q failed: %w", info.Version.Number, err) | ||
| } | ||
|
|
||
| return info.Version.Number, int(version.Major()), nil |
There was a problem hiding this comment.
Can we please simply return the uint64 instead of casting this?
| if statusCode <= 299 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
So we do accept 1xx codes as good?
| if interval <= 0 { | ||
| return | ||
| } |
There was a problem hiding this comment.
Can we please check this outside of this function?!
| ctx, cancel := context.WithCancel(context.Background()) | ||
|
|
||
| var wg sync.WaitGroup | ||
| wg.Add(1) | ||
| go func() { |
There was a problem hiding this comment.
I would prefer to do the goroutine setup on the caller side and pass the context in here. This saves the returned function and makes it clear that the client runs another thread...
|
|
||
| // startDiscovery runs node discovery immediately and repeats it when the interval is positive. | ||
| // The returned function stops the loop and waits for any active call to return. | ||
| func startDiscovery(log telegraf.Logger, interval time.Duration, discover func(context.Context) error) func() { |
There was a problem hiding this comment.
Can you please make log the last argument?! We usually do this in other places...
| if cfg.enableSniffer { | ||
| cfg.httpClient.CloseIdleConnections() | ||
| return nil, errors.New("enable_sniffer is not supported by the official ElasticSearch v5 client") | ||
| } |
There was a problem hiding this comment.
Can we please check this on the caller side?
| } | ||
| var result map[string]interface{} | ||
| if err := json.NewDecoder(res.Body).Decode(&result); err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Can we please prefix the error to easier find the spot when we get a bug-report?
| return nil, err | |
| return nil, fmt.Errorf("decoding message body failed: %w", err) |
| } | ||
| } | ||
|
|
||
| func (e *ElasticsearchQuery) Start(telegraf.Accumulator) error { |
There was a problem hiding this comment.
Can we please merge this with newClient?!
| QueryPeriod: config.Duration(time.Second * 600), | ||
| }, | ||
| expected: "Error 404 (Not Found): no such index", | ||
| expected: "received error 404 (Not Found): no such index", |
There was a problem hiding this comment.
Please revert. The function should use require.ErrorContains which can perfectly handle prefixed errors. This makes the test more robust if we change the prefix later.
Same below.
| require.ErrorContains(t, err, `parsing server version "invalid" failed`) | ||
| require.NotContains(t, err.Error(), "getting server version failed") |
There was a problem hiding this comment.
Can this really occur? I thought those code paths are mutually exclusive.
Summary
olivere/elastic.Node discovery caveats
As results of official clients limitations:
enable_sniffer = truewill now fail explicitly because the official v5 client does not support node discovery.DiscoverNodes()has no context support. Shutdown waits for it to finish, bounded by the configured HTTP timeout (5s by default).Checklist
Related issues
resolves #19212