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

[metricbeat] implement Close() for docker metricsets #11294

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Mar 18, 2019

More info at #11266

This is something I noticed while looking over the inconsistencies in how various metricsets do or do not implement a Close().

@ruflin Mentioned that there may be some weird behavior around docker and closing connections. As of now this looks straightforward, but we can check to see if something crops up during CI.

@fearful-symmetry fearful-symmetry added bug Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 18, 2019
@fearful-symmetry fearful-symmetry requested review from ruflin and a team March 18, 2019 18:36
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. Could you add a changelog entry? I suggest we also backport this as it could mean a memory leak.

@fearful-symmetry fearful-symmetry requested review from a team as code owners March 19, 2019 16:01
@fearful-symmetry fearful-symmetry removed request for a team March 19, 2019 16:06
@jsoriano
Copy link
Member

I think we should implement Close() in all docker metricsets that use the docker client in the same way.

Let's do all of them in this PR so we keep them together also for backporting as they are going to be the same thing.

@jsoriano jsoriano added review containers Related to containers use case labels Mar 19, 2019
@fearful-symmetry fearful-symmetry changed the title [metricbeat] implement Close() for the docker container metricset [metricbeat] implement Close() for docker metricsets Mar 19, 2019
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

LGTM, but before merging update the changelog entry now that not only the container metricset is changed.

CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
Co-Authored-By: fearful-symmetry <8418476+fearful-symmetry@users.noreply.github.com>
@fearful-symmetry fearful-symmetry merged commit 57f8b5c into elastic:master Mar 19, 2019
@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

@fearful-symmetry We should also backport this to 7.0 as it's a bug.

fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Mar 20, 2019
* implement Close() for docker metricsets

(cherry picked from commit 57f8b5c)
fearful-symmetry added a commit that referenced this pull request Mar 21, 2019
* implement Close() for docker metricsets

(cherry picked from commit 57f8b5c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug containers Related to containers use case Metricbeat Metricbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants