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.

Sign up for GitHub

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

Jump to bottom

Create a metricset for swap and load #2072

Closed
wants to merge 5 commits into from
Closed

Create a metricset for swap and load #2072

wants to merge 5 commits into from

Conversation

monicasarbu
Copy link
Contributor

@monicasarbu monicasarbu commented Jul 20, 2016 •
edited
Loading

This PR includes the following changes:

  • Create a swap metricset instead of including swap information in the memory metricset
  • Create a load metricset instead of including load information in the cpu metricset

This PR includes few breaking changes:

  • system.memory.actual.free is replace with system.memory.available
  • system.memory.actual.used is not needed as it can calculated from system.memory.total - system.memory.available
  • the configuration file changes, meaning that you need to add swap and load if you want to export these data. They are not part of memory and cpu anymore.

cc-ed @ruflin @andrewkroh

@monicasarbu monicasarbu added in progress Pull request is currently in progress. review labels Jul 20, 2016
@monicasarbu monicasarbu force-pushed the use_gopsutil_for_mem_and_swap branch from 6fc7e26 to 0e33430 Compare July 20, 2016 10:24
@monicasarbu monicasarbu changed the title Use gopsutil for memory and swap and export extra options Create a metricset for swap and load Jul 20, 2016
@monicasarbu monicasarbu removed the in progress Pull request is currently in progress. label Jul 20, 2016
ruflin
ruflin reviewed Jul 20, 2016
View reviewed changes
metricbeat/module/system/load/_meta/docs.asciidoc
@@ -0,0 +1,11 @@
=== System CPU Metricset
Copy link
Member

Choose a reason for hiding this comment

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

Should be load?

@ruflin
Copy link
Member

ruflin commented Jul 20, 2016

I really like this change as I think it makes finding the right metrics easier and also simplifies the code.

andrewkroh
andrewkroh reviewed Jul 20, 2016
View reviewed changes
metricbeat/docs/fields.asciidoc


[float]
== swap Fields
=== system.memory.active
Copy link
Member

Choose a reason for hiding this comment

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

I would like to have a more detailed description for some of these fields and possibly how the data is collected (e.g. read from /proc/meminfo or same values reported by vmstat -a).

Active — The total amount of buffer or page cache memory, in kilobytes, that is in active use. This is memory that has been recently used and is usually not reclaimed for other purposes.

Inactive — The total amount of buffer or page cache memory, in kilobytes, that are free and available. This is memory that has not been recently used and can be reclaimed for other purposes.

@andrewkroh
Copy link
Member

I think this is going to break openbsd support which was added to gosigar.

@monicasarbu monicasarbu force-pushed the use_gopsutil_for_mem_and_swap branch from df23655 to 0ee0318 Compare July 20, 2016 13:59
monicasarbu
monicasarbu reviewed Jul 20, 2016
View reviewed changes
metricbeat/tests/system/test_system.py
@@ -26,7 +28,10 @@

SYSTEM_FSSTAT_FIELDS = ["count", "total_files", "total_size"]

SYSTEM_MEMORY_FIELDS = ["swap", "actual", "free", "total", "used.bytes", "used.pct"]
SYSTEM_MEMORY_FIELDS = ["available", "free", "total", "used.bytes", "used.pct", "active", "inactive", "wired",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to remove "active", "inactive" ...

@monicasarbu monicasarbu mentioned this pull request Jul 20, 2016
Closed
andrewkroh
andrewkroh reviewed Jul 21, 2016
View reviewed changes
metricbeat/etc/fields.yml
description: >
Actual available memory. This value is the "free" memory plus the memory used for disk caches and
buffers. Available only on Unix.
Free memory. For a human readable value use Available.
Copy link
Member

Choose a reason for hiding this comment

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

"human readable" means to me that it's the same value formatted with units like MB or GB. It's like df -h.

Consider saying this "The total amount of free memory in bytes. This value does not include memory consumed by system caches and buffers (see system.memory.available)."

@andrewkroh
Copy link
Member

The system/load package must have a doc.go file. Otherwise metricbeat will fail to build on operating systems that don't support load because list.go imports what appears to be an empty package. This is why the travis build failed. https://travis-ci.org/elastic/beats/jobs/146202233

@andrewkroh
Copy link
Member

I'm not fully convinced that swap is completely separate concept from memory to necessitate it's own metricset. For example, most things in the operating system group all of this data together under the umbrella term or "memory" like /proc/meminfo and /sys/fs/cgroup/memory/memory.stat.

@monicasarbu
Copy link
Contributor Author

@andrewkroh I agree swap should be in the same document with memory. That's why my initial PR was to have system.memory and system.swap in the same document, but this is not possible with the current Metricbeat architecture. So, I am closing this PR as it seems that the best possible option is the current one.

@ruflin
Copy link
Member

ruflin commented Jul 25, 2016

@monicasarbu @andrewkroh What about the load part of this PR? I still like that load is a separate metricset as it is a summary over time.

@andrewkroh
Copy link
Member

Yeah, I also liked the load separation. The memory field renaming is also nice.

@monicasarbu
Copy link
Contributor Author

I created another PR to add theload metricset and change the name of the memory exported fields. #2101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@monicasarbu @ruflin @andrewkroh