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

Reporting and showing cgroup-based metrics #291

Closed
eyalkoren opened this issue Jul 1, 2020 · 13 comments
Closed

Reporting and showing cgroup-based metrics #291

eyalkoren opened this issue Jul 1, 2020 · 13 comments
Labels
apm-agents enhancement New feature or request poll
Milestone

Comments

@eyalkoren
Copy link
Contributor

eyalkoren commented Jul 1, 2020

Description of the issue

APM agents currently send system metrics that are aligned with Metricbeat's metricset keys, as well as values. These cover system. metricsets and some specific platform-related metrics (see Java agent documentation for example).
However, these system metrics are inaccurate when monitoring containers. The most obvious miscalculation comes from the fact that agents currently collect host total memory rather than the effective cgroup limitation, but there are also considerable differences in the used bytes, depending on how they are retrieved, as well as CPU usage per cgroup quota.

Proposed solution

Introducing new cgroup metrics

As a first step, the new metrics will include:

  • system.process.cgroup.memory.mem.limit.bytes
  • system.process.cgroup.memory.mem.usage.bytes

Both are optional.
Both are numeric representing number of bytes.
When not available, these metrics should not be sent.

In the future, we may extend to collect and show additional memory metrics, as well as cpu metrics.

APM UI

System memory usage values will be calculated based on cgroup metrics if such are available, using mem.usage.bytes/mem.limit.bytes. Otherwise, use the existing system.memory metrics.

NOTE: whenever a cgroup is not explicitly limited in memory, the limit read from the corresponding file may be set to 9223372036854771712 (equivalent to 0x7ffffffffffff000), which basically means unlimited.
Agents conforming to the spec should not send this value (they should omit the max cgroup metric in such case)..

Formalizing that in pseudocode:

var total = system.process.cgroup.memory.mem.limit.bytes;
if (total == NA) {  
  total = system.memory.total;
}
var used = system.process.cgroup.memory.mem.usage.bytes;
if (used == NA) {
  used = system.memory.total - system.memory.actual.free;
} 
var usage = used / total;

Agent implementation details

https://github.com/elastic/apm/blob/master/specs/agents/metrics.md#cgroup-metrics

Related issues

Component Link to issue
Agents #292
APM Server elastic/apm-server#4070
APM UI elastic/kibana#69679
@eyalkoren eyalkoren added enhancement New feature or request poll labels Jul 1, 2020
@beniwohli
Copy link
Contributor

What speaks against defaulting to /sys/fs/cgroup/memory and only resort to parsing mountinfo if that file doesn't exist? Is there a realistic chance that /sys/fs/cgroup/memory exists but is the wrong file?

@eyalkoren
Copy link
Contributor Author

A thought: do you think the special handling of the unlimited value should be done by UI instead? @elastic/apm-ui would that be a problem?

@eyalkoren
Copy link
Contributor Author

What speaks against defaulting to /sys/fs/cgroup/memory and only resort to parsing mountinfo if that file doesn't exist? Is there a realistic chance that /sys/fs/cgroup/memory exists but is the wrong file?

Nothing, it's either one way or the other 🙂 . It's the same effort in terms of implementation and negligible in terms of performance (assuming done only once), so I assume could be either way.

@beniwohli
Copy link
Contributor

Nothing, it's either one way or the other 🙂 . It's the same effort in terms of implementation and negligible in terms of performance (assuming done only once), so I assume could be either way.

My thought was that because you pointed out that the cgroup-v1 regex might need some iteration and we haven't one for cgroup-v2, the chance of false positives is lower if we try the path that the majority of systems will use anyway first. However, now I'm not sure if a false positive is even possible.

@eyalkoren
Copy link
Contributor Author

However, now I'm not sure if a false positive is even possible.

It depends on what you define as false positives. Extracting non-valid path based on our regex is very likely, at least as we start. However, I don't consider those as false positives, as we should check that such path exists and contains the expected files.
Like you, I'm not sure if both can exist and contradict each other (I'm guessing not, but it's really a guess)

@sorenlouv
Copy link
Member

sorenlouv commented Jul 1, 2020

A thought: do you think the special handling of the unlimited value should be done by UI instead? @elastic/apm-ui would that be a problem?

Probably fine but not 100% how the ui should handle this. Afaiu mem.limit.bytes might be unbounded and thus set to 9223372036854771712. The ui should detect this and change it to another value. What value is this?

My mental model in code:

const unboundedMemSize = 9223372036854771712
if ( mem.limit.bytes === unboundedMemSize ) {
  mem.limit.bytes = ??
}

@felixbarny
Copy link
Member

Could we convert this issue into a spec PR? See also #192.

@eyalkoren
Copy link
Contributor Author

eyalkoren commented Jul 1, 2020

@sqren my mental model is something like:

var total = system.process.cgroup.memory.mem.limit.bytes;
if (total == 9223372036854771712 || total == NA) {  
  total = system.memory.total;
}
var used = system.process.cgroup.memory.mem.usage.bytes;
if (used == NA) {
  used = system.memory.total - system.memory.actual.free;
} else if (system.process.cgroup.memory.stats.inactive_file.bytes != NA) {
  used -= system.process.cgroup.memory.stats.inactive_file.bytes;
}
var usage = used / total;

Makes sense?

@eyalkoren
Copy link
Contributor Author

Could we convert this issue into a spec PR? See also #192.

Sure. Since there are discussions already ongoing here, let's conclude them and then create a corresponding spec PR.

@eyalkoren
Copy link
Contributor Author

UPDATE: after talking to @sqren offline, we agreed that UI will do the special handling for the unlimited value, which means - agents should ignore this case and always report the limit as it is read from the file.
I updated the proposal in the issue description accordingly.
cc @elastic/apm-agent-devs

@eyalkoren
Copy link
Contributor Author

UPDATE II: @elastic/apm-agent-devs I just learned that in cgroup v2 the special value used for representing unlimited memory for the cgroup is represented by a string - max. Therefore, agents should be aware and avoid from sending the system.process.cgroup.memory.mem.limit.bytes metric when that's the case.
I updated the description above and will update the spec accordingly.

@felixbarny
Copy link
Member

Closing as there are follow-up issues for each component

@eyalkoren
Copy link
Contributor Author

UPDATE - removed system.process.cgroup.memory.stats.inactive_file.bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm-agents enhancement New feature or request poll
Projects
None yet
Development

No branches or pull requests

5 participants