Used memory field corrected (#4461)

[amandahla]

Change for correct the used memory field.

[elasticmachine]

Can one of the admins verify this patch?

[elasticmachine]

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically on build-eu-00. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

[tsg]

jenkins, test it

[andrewkroh]

Hi @amandahla, is there a test case you can add for this? It would be nice to have something in place that demonstrates this fixes the issue and prevents future regression.

And BTW if you put "Closes #4461" in the commit message then the issue will automatically be closed when this merges.

[amandahla]

@andrewkroh hs.Summary.QuickStats.OverallMemoryUsage is a int32 value for memory usage in MB.
When I multiply twice to get bytes numbers without the conversion, it exceeds the int32 maximum and gives a wrong number. We just get this when we saw weird values on dashboard. So I saw that I must convert to int64 to fit the right value.
I don't know how can I test when it's necessary a bigger variable type. Any suggestions?

Thanks for the tip about close issues!

[andrewkroh]

Any suggestions?

I recommend splitting up the logic from Fetch into smaller functions that can be easily unit tested (without requiring the simulator).

[amandahla]

@andrewkroh I'm not sure if I get the idea. Is something like this?

[andrewkroh]

Yeah, sort of. I guess I was thinking more like this (or some other break down of the existing logic to make it unit testable without the use of the data simulator).

func (m *MetricSet) Fetch() ([]common.MapStr, error) {
    ...
    for _, hs := range hst {
        events = append(events, buildEvent(hs))
    }
    ...
}
func buildEvent(hs mo.HostSystem) common.MapStr { ... }

func TestBuildEvent(t *testing.T) {
   var hs mo.HostSystem
   hs.Summary.Hardware.MemorySize = math.MaxInt32
   // set other values

   event := buildEvent(hs)

   // assert correct output w/o overflow
}

[amandahla]

@andrewkroh Another try :)

[andrewkroh]

Looks like you got the idea. 👍

[andrewkroh]

It looks like this file is only used for testing. If so it should be named with the _test.go suffix so that it is only compiled and used in tests. But I would probably just move the HostSystemTest definition into the data_test.go.

[andrewkroh]

It looks like you could have got away with a much smaller set of stub data for your testing purposes because eventMapping only needs a few of the fields.

[andrewkroh]

FYI It should be possible to write this as

value, _ := event.Get("cpu.used.mhz")
assert.EqualValues(t, 67, value)

Personally, I find using the dotted field notational easier to work with.