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] Add windows memory data to docker/memory #12172

Conversation

fearful-symmetry
Copy link
Contributor

@fearful-symmetry fearful-symmetry commented May 10, 2019

Leaving this as a draft since I'm not 100% sure how to handle vastly different stats across various operating systems. Will probably add more commits later as I tinker around.

This addresses #11971 by adding the memory stats that are only available on windows. The change itself is fairly simple, but I have no practical experience with docker/go on windows, so I would appreciate help from any other folks. I added @narph to the reviewers list as well.

@fearful-symmetry fearful-symmetry added :Windows Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels May 10, 2019
@fearful-symmetry fearful-symmetry requested review from narph and a team May 10, 2019 17:48
@fearful-symmetry fearful-symmetry self-assigned this May 10, 2019
@fearful-symmetry fearful-symmetry changed the title [metricbeat] WIP of adding windows memory data [metricbeat] Add windows memory data to docker/memory May 10, 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.

This looks quite good to me if we cannot consider that Windows metrics are more or less equivalent to Linux ones.

@@ -66,5 +70,9 @@ func (s *MemoryService) getMemoryStats(myRawStat docker.Stat, dedot bool) Memory
TotalRssP: float64(totalRSS) / float64(myRawStat.Stats.MemoryStats.Limit),
Usage: myRawStat.Stats.MemoryStats.Usage,
UsageP: float64(myRawStat.Stats.MemoryStats.Usage) / float64(myRawStat.Stats.MemoryStats.Limit),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these percentages are NaN because of 0/0 on Windows, we should probably check for these limits not being 0. Though it wouldn't be so important now if we send different fields for Windows.

"pct": memoryData.UsageP,
"max": memoryData.MaxUsage,
},
}
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with this approach if we cannot consider these metrics equivalent to others on Linux. The private working set could be similar to the RSS, and commit and commit peak could be similar to usage and max usage, but not sure.
Probably @narph or @andrewkroh can shed more light about this.
If they are equivalent or similar enough we should send them in the same fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit - memory space the application has reserved for use, so this will be allocated but not necessarily used. (This is composed of main memory (RAM) and disk (pagefiles).)
CommitPeak - the highest amount that the commit has reached since the operating system was last started or session.
PrivateWorkingSet - the amount of memory used by a process.
Not sure the memory stats are matching here so I would go for separate naming, we might want to look into that as well for the other memory related metricsets

fields = common.MapStr{
"commit": memoryData.Commit,
"commit_peak": memoryData.CommitPeak,
"private_working_set": memoryData.PrivateWorkingSet,
Copy link
Member

Choose a reason for hiding this comment

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

Follow the structure of Linux metrics, in case some day docker supports limits in Windows and we can calculate percentages.

Copy link
Contributor

Choose a reason for hiding this comment

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

++

@fearful-symmetry
Copy link
Contributor Author

Adjusted things a tad so now we have commited bytes in usage at least.

Regarding @jsoriano 's comments: I agree that we should make these more consistent, and that things like rss on linux and Private Working Set on Windows are probably more or less equivalent. My concern is finding some sort of naming scheme that doesn't conflate the two, as technically RSS is a linux concept, not a windows one. I worry that if we're not careful and we just cram metrics together, we might confuse people with linux-sounding Windows metrics and so on. I adjusted the format slightly so windows has usage now, but I would like input from some windows experts to see what else we can do to smooth out the naming.

@fearful-symmetry fearful-symmetry force-pushed the docker-windows-add-memory-data branch from 2ab8496 to bd22180 Compare May 13, 2019 19:48
@jsoriano
Copy link
Member

I worry that if we're not careful and we just cram metrics together, we might confuse people with linux-sounding Windows metrics and so on.

Yeah, that's actually a point to discuss. The good thing of having all the equivalent metrics in the same fields is that you can build dashboards or queries showing this information together more easily. One option can be to keep OS-specific fields, but also duplicate these values into common fields. This could be added afterwars in any case.

@fearful-symmetry
Copy link
Contributor Author

One option can be to keep OS-specific fields, but also duplicate these values into common fields. This could be added afterwars in any case.

That's one option, yah! We did something similar with the RAID metricset, where we had a detail section, and a normalized section for RAID states.

@narph
Copy link
Contributor

narph commented May 23, 2019

@fearful-symmetry , I have not commented on the initial naming as I found it to be clear enough.

"commit":              memoryData.Commit,
"commit_peak":         memoryData.CommitPeak,
"private_working_set": memoryData.PrivateWorkingSet,

You could go for Commit size instead of Commit here.

I would stir away from usage or memory usage when talking about private working set as there are more types here (of memory usage): working set, private working set, working set will also include the memory shared by other processes.
https://answers.microsoft.com/en-us/windows/forum/windows_10-performance/physical-and-virtual-memory-in-windows-10/e36fb5bc-9ac8-49af-951c-e7d39b979938
http://blogs.microsoft.co.il/sasha/2016/01/05/windows-process-memory-usage-demystified/

@jsoriano, Is there a way we can store only the os specific information and utilize the field alias to create a common visualization/dashboard for the user instead of going for duplication? (I have not tried this)

@fearful-symmetry
Copy link
Contributor Author

Thanks for your added input @narph.
I definitely like the alias idea, but I have absolutely no idea how to do that.

@jsoriano
Copy link
Member

jsoriano commented May 24, 2019

@jsoriano, Is there a way we can store only the os specific information and utilize the field alias to create a common visualization/dashboard for the user instead of going for duplication? (I have not tried this)

I don't think we can use the alias field for this. Aliases are defined in the index mapping (not per document), and can only be defined in a 1 to 1 way, so we couldn't create an alias pointing to several OS-specific metrics.

I'd keep both OS-specific metrics in the events by now as they are different things (thanks @narph for the clarifications!), and we can evaluate later what to do if we create visualizations for this. Maybe this can be solved in the visualization or the dashboard itself.

@fearful-symmetry
Copy link
Contributor Author

fearful-symmetry commented May 24, 2019

I'd keep both OS-specific metrics in the events by now

@jsoriano So we're good as-is? Should I take this out of draft?

@jsoriano
Copy link
Member

I'd keep both OS-specific metrics in the events by now

@jsoriano So we're good as-is? Should I take this out of draft?

Yeah, I think this can go for review. Only let me know what you think about this comment https://github.com/elastic/beats/pull/12172/files#r283109849

It would be to change the metrics to something like this:

			"usage": common.MapStr{
				"commit": common.MapStr{
                                    "total": memoryData.Commit,
				    "peak":  memoryData.CommitPeak,
                                },
			},
			"private_working_set": common.MapStr{
                            "total": memoryData.PrivateWorkingSet,
                        },

In case some day we add also percentages or other aggregations.

@fearful-symmetry
Copy link
Contributor Author

Ahh, alright, I wasn't 100% sure what you meant by that, so I figured the usage dict was what you meant. Yah, I like that idea.

@fearful-symmetry
Copy link
Contributor Author

Okay, I think I'm confusing myself with the fields.yml format, but it should be good now.

@fearful-symmetry fearful-symmetry marked this pull request as ready for review May 24, 2019 18:58
@fearful-symmetry fearful-symmetry requested a review from a team as a code owner May 24, 2019 18:58
@fearful-symmetry fearful-symmetry requested a review from a team May 24, 2019 18:58
@jsoriano
Copy link
Member

@fearful-symmetry are you going to do the change for commit and commit peak too?

It would be to change the metrics to something like this:

			"usage": common.MapStr{
				"commit": common.MapStr{
                                    "total": memoryData.Commit,
				    "peak":  memoryData.CommitPeak,
                                },
			},

@fearful-symmetry
Copy link
Contributor Author

Oh, is that how we want to do it? I figured we could just use the existing pct type in the commit object.

@fearful-symmetry
Copy link
Contributor Author

Okay. Hopefully this makes sense now.

jsoriano
jsoriano previously approved these changes May 28, 2019
@jsoriano jsoriano dismissed their stale review May 28, 2019 17:04

We are discussing offline about the proper location of the commit metrics

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.

A couple of comments on documentation, for the rest it LGTM, sorry for the back and forth on field names!

metricbeat/module/docker/memory/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/docker/memory/_meta/fields.yml Outdated Show resolved Hide resolved
@fearful-symmetry fearful-symmetry merged commit 1c75c66 into elastic:master May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat Team:Integrations Label for the Integrations team :Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants