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

Add support for huge pages on Linux #6436

Merged
merged 2 commits into from
Mar 26, 2018
Merged

Conversation

jsoriano
Copy link
Member

Add support for huge pages on Linux, fixes #6351, requires elastic/gosigar#97

@jsoriano jsoriano added enhancement in progress Pull request is currently in progress. review libbeat Metricbeat Metricbeat labels Feb 21, 2018
s.UsedPercent = common.Round(perc, common.DefaultDecimalPlacesCount)
}

func GetHugeTLBPagesEvent(s *HugeTLBPagesStat) common.MapStr {

Choose a reason for hiding this comment

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

exported function GetHugeTLBPagesEvent should have comment or be unexported

return nil, err
}

func AddHugeTLBPagesPercentage(s *HugeTLBPagesStat) {

Choose a reason for hiding this comment

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

exported function AddHugeTLBPagesPercentage should have comment or be unexported

UsedPercent float64 `json:"used_p"`
}

func GetHugeTLBPages() (*HugeTLBPagesStat, error) {

Choose a reason for hiding this comment

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

exported function GetHugeTLBPages should have comment or be unexported

@jsoriano jsoriano force-pushed the huge-tlb-pages branch 2 times, most recently from e0d3c48 to e917488 Compare February 21, 2018 16:35
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

This is looking good, you will need to update fields.yml to configure mapping for the new fields: https://www.elastic.co/guide/en/beats/devguide/current/metricset-details.html#_fields_yml

}

// GetHugeTLBPagesEvent returns the event created from huge page usage metrics
func GetHugeTLBPagesEvent(s *HugeTLBPagesStat) common.MapStr {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function is not used?

Copy link
Member Author

@jsoriano jsoriano Feb 21, 2018

Choose a reason for hiding this comment

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

Umm, there are other similar GetFooEvent functions here, and indeed they are not used in the beats repo, I though they were exported to be used somewhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

@exekias WDYT? Should we keep these GetFooEvent functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed new unused function, old ones are left there by now.

@exekias
Copy link
Contributor

exekias commented Feb 21, 2018

Also have a look to system tests, you may want to check fields are in place in Linux: https://github.com/elastic/beats/blob/master/metricbeat/tests/system/test_system.py

@exekias
Copy link
Contributor

exekias commented Feb 21, 2018

Failing tests should be fixed by make update, that will format the code to the standard and update NOTICE.txt

@jsoriano jsoriano force-pushed the huge-tlb-pages branch 3 times, most recently from 426593a to f2472b7 Compare February 22, 2018 17:44
@exekias
Copy link
Contributor

exekias commented Mar 10, 2018

I saw elastic/gosigar#97 was merged 🎉. Let's move this forward once we get a new tag version of it

@jsoriano
Copy link
Member Author

gosigar 0.9.0 released, I will go on with this PR using this version.

@jsoriano jsoriano force-pushed the huge-tlb-pages branch 3 times, most recently from c93f90f to 4ca00ee Compare March 22, 2018 12:13
@jsoriano jsoriano removed the in progress Pull request is currently in progress. label Mar 23, 2018
@ruflin
Copy link
Contributor

ruflin commented Mar 26, 2018

Can you add a changelog entry?

@jsoriano
Copy link
Member Author

jsoriano commented Mar 26, 2018

Changelog entry added.
Please wait a bit before merging this, I'd like to try to reproduce this problem seen upgrading to gosigar 0.9.0: https://discuss.elastic.co/t/metricbeat-5-3-0-reporting-strange-memory-value/124825

@jsoriano jsoriano added in progress Pull request is currently in progress. and removed in progress Pull request is currently in progress. labels Mar 26, 2018
@jsoriano
Copy link
Member Author

We can continue merging this, it seems that the problem with gosigar was not introduced in 0.9.0, it was already reported before (#6271)

@exekias exekias merged commit 988ab7b into elastic:master Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include hugepages metrics in metricbeat
4 participants