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][Docker] Docker cpu norms #13695

Merged
merged 8 commits into from
Sep 18, 2019

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Sep 16, 2019

This PR implements the proposal for docker.cpu.*.norm.pct fields described on #13343.

How to test it

  1. Enable Docker Module and cpu metricset specifically.
  2. Run Metricbeat and check that all the new normalised fields are present:
  • kernel.norm.pct
  • system.norm.pct
  • user.norm.pct
  • total.norm.pct
  • core.*.norm.pct

@ChrsMark ChrsMark added enhancement in progress Pull request is currently in progress. containers Related to containers use case Team:Integrations Label for the Integrations team labels Sep 16, 2019
@ChrsMark ChrsMark requested a review from a team as a code owner September 16, 2019 12:47
@ChrsMark ChrsMark self-assigned this Sep 16, 2019
Copy link
Contributor

@fearful-symmetry fearful-symmetry left a comment

Choose a reason for hiding this comment

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

Hey, can we get Godocs for the rest of the exported methods in helper.go ? Surprised hound didn't complain.

@ChrsMark
Copy link
Member Author

Hey, can we get Godocs for the rest of the exported methods in helper.go ? Surprised hound didn't complain.

👍 sure! thank you!

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added [zube]: In Review review and removed in progress Pull request is currently in progress. labels Sep 18, 2019
@zube zube bot added in progress Pull request is currently in progress. [zube]: In Progress review and removed [zube]: In Progress review [zube]: In Review in progress Pull request is currently in progress. labels Sep 18, 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.

In principle apart of the small comment it LGTM.

I have a couple of questions before merging:

In the system modules, the cpu metrics collected are optional and managed by the cpu.metrics setting. It can be useful if more methods are added, though I think that not really needed because the overhead doesn't look so big. Did you consider doing something similar here?

Not to be done in this PR, but I think that it could be good to expose the data to calculate percentages in the events, so custom percentages can be calculated on query time. Now we have something like the number of cpus of the host in docker.cpu.system.pct, but it is a bit counterintuitive, should we add a explicit field for that? like docker.cpu.online_cpus?

}
//if len(stats.CPUStats.CPUUsage.PercpuUsage) == 0 {
// u.cpus = 1
//}
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented out code.

Copy link
Member Author

Choose a reason for hiding this comment

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

🙈 thank you for catching this!

@jsoriano
Copy link
Member

Also a changelog entry will be needed.

@ChrsMark
Copy link
Member Author

In principle apart of the small comment it LGTM.

Thank you for reviewing this Jaime!

I have a couple of questions before merging:

In the system modules, the cpu metrics collected are optional and managed by the cpu.metrics setting. It can be useful if more methods are added, though I think that not really needed because the overhead doesn't look so big. Did you consider doing something similar here?

Not to be done in this PR, but I think that it could be good to expose the data to calculate percentages in the events, so custom percentages can be calculated on query time. Now we have something like the number of cpus of the host in docker.cpu.system.pct, but it is a bit counterintuitive, should we add a explicit field for that? like docker.cpu.online_cpus?

Yeap I was thinking about this too, however I didn't add this yet since I was not sure enough because it was not already there for pct and ticks.
I'm really interested in having this extension in this metriset too and I could work on this. We could open another issue to discuss the specs, WDYT?

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark requested a review from jsoriano September 18, 2019 11:10
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.

Ok, let's go on with this and please open another issue to add metrics about limited resources. Apart of adding metrics about the available CPUs, we could also check if we are collecting metrics about available memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case enhancement review Team:Integrations Label for the Integrations team v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants