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] check for a valid limit before we process a memory event #11676

Merged
merged 2 commits into from
Apr 9, 2019

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented Apr 5, 2019

This fixes #11283

It appears that we can get a stat object back when a container is in some odd in between state where it's not actually started. During this time, all the memory values we're using are zero, and we get NaNs from the division operations:

"memory":common.MapStr{"limit":0x0, "rss":common.MapStr{"total":0x0, "pct":NaN}, "usage":common.MapStr{"pct":NaN, "max":0x0, "total":0x0}, "fail":common.MapStr{"count":0x0}}}

The docker source states that Limit will only be 0 if a container isn't started, so we use that to check if we have a valid event. We can also process the event and just wrap the division in if blocks, but I liked the idea of skipping the event if we have zero'ed out data.

@fearful-symmetry fearful-symmetry added bug module Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Apr 5, 2019
@fearful-symmetry fearful-symmetry self-assigned this Apr 5, 2019
@fearful-symmetry fearful-symmetry requested a review from a team April 5, 2019 19:37
@odacremolbap
Copy link
Contributor

+1
And awesome adding comments to existing funcs, thanks for that.
I guess that it is not a real race, docker exposes the endpoints as soon as the container object is created, and we are polling before there is any data there.

It would be probably very expensive to GET for the container status at every request, so checking for the value that breaks our code sounds right to me.

Not needed, but a bit more awesomeness and love if setting a proper variable name to
https://github.com/elastic/beats/blob/0d9bdcf21f41aa41598a60a8da808a245be14e01/metricbeat/module/docker/memory/helper.go#L43

something like

	for _, containerStats := range containersStats {

@fearful-symmetry
Copy link
Contributor Author

@odacremolbap Yah, I kinda gave those variable name the side-eye earlier too.

@@ -22,6 +22,7 @@ import (
"github.com/elastic/beats/metricbeat/module/docker"
)

//MemoryData contains parsed container memory info
Copy link
Contributor

Choose a reason for hiding this comment

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

a space here would be great :-) "// MemoryData..." and same in line 38

@ruflin
Copy link
Member

ruflin commented Apr 8, 2019

Can you add a changelog entry? Also added needs_backport as I think we should fix this too in older versions.

@ruflin ruflin added needs_backport PR is waiting to be backported to other branches. [zube]: In Review labels Apr 8, 2019
@fearful-symmetry
Copy link
Contributor Author

Looks like coredns is failing here now too:
Exception: Timeout while waiting for healthy docker-compose services: coredns

@ruflin
Copy link
Member

ruflin commented Apr 9, 2019

Let's try to rebase.

@fearful-symmetry fearful-symmetry merged commit 97aec9b into elastic:master Apr 9, 2019
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Apr 11, 2019
…elastic#11676)

* check for a valid limit before we process a memory event

(cherry picked from commit 97aec9b)
fearful-symmetry added a commit that referenced this pull request Apr 11, 2019
…a memory event (#11728)

* [metricbeat] check for a valid limit before we process a memory event (#11676)
fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Apr 15, 2019
…elastic#11676)

* check for a valid limit before we process a memory event

(cherry picked from commit 97aec9b)
fearful-symmetry added a commit that referenced this pull request Apr 15, 2019
…a memory event (#11726)

* [metricbeat] check for a valid limit before we process a memory event (#11676)
@jsoriano
Copy link
Member

It seems it was already backported, I am removing the needs_backport label.

@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Metricbeat Metricbeat module Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to publish events: unsupported float value: NaN / Broken Error handling
6 participants