-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[metricbeat] implement Close() for docker metricsets #11294
Conversation
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.
LGTM. Could you add a changelog entry? I suggest we also backport this as it could mean a memory leak.
ea4374a
to
4129418
Compare
I think we should implement 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. |
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.
LGTM, but before merging update the changelog entry now that not only the container metricset is changed.
Co-Authored-By: fearful-symmetry <8418476+fearful-symmetry@users.noreply.github.com>
@fearful-symmetry We should also backport this to 7.0 as it's a bug. |
* implement Close() for docker metricsets (cherry picked from commit 57f8b5c)
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.