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 to set gc percentage #25394

Merged
merged 7 commits into from
May 2, 2021
Merged

Conversation

urso
Copy link

@urso urso commented Apr 28, 2021

  • Bug

What does this PR do?

Introduce new setting to Beats: gc_percentage
The new setting sets GOGC, which configures the next gc limit. The
default value is 100. Setting the percentage less to 100 makes the Beats
trigger collection more often, due to a less next gc limit.

In Agent we add per process type gc limit environment variables:
APMSERVER_GOGC, FILEBEAT_GOGC, and METRICBEAT_GOGC.
These can be used to fine tune gc percentage limit per sub process type

Why is it important?

Allow users to set gc percentage.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

Set gc_percentage in the configuration file or via -E. The Beat will print a log message (info level) on startup.

Introduce new setting to Beats: gc_percentage
The new setting sets GOGC, which configures the next gc limit. The
default value is 100. Setting the percentage less to 100 makes the Beats
trigger collection more often, due to a less next gc limit.

In Agent we add per process type gc limit environment variables:
APMSERVER_GOGC, FILEBEAT_GOGC, and METRICBEAT_GOGC.
These can be used to fine tune gc percentage limit per sub process type
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 28, 2021
@urso urso added the Team:Elastic-Agent Label for the Agent team label Apr 28, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 28, 2021
"-E", "management.enabled=true",
"-E", "management.mode=x-pack-fleet",
"-E", "apm-server.data_streams.enabled=true"
"-E", "gc_percent=${APMSERVER_GOGC:100}"
Copy link
Author

Choose a reason for hiding this comment

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

This works because libbeat merges all -E flags into the configuration. The environment variable with default will be evaluated by libbeat.

Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

Looks good.

runtime.GOMAXPROCS(maxProcs)
}
if gcPercent := b.Config.GCPercent; gcPercent > 0 {
logp.Info("Set gc percentage to: %v", gcPercent)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth logging if the value is 100? Being that its the default.

Copy link
Author

Choose a reason for hiding this comment

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

We only log if it is configured. Might be helpful to see all message when we have to compare configs with logs output in support cases.

Copy link
Member

Choose a reason for hiding this comment

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

For the logs that are only logged once on startup (assuming this is correct) we should rather log too much then too little. Makes debugging easier.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 28, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #25394 updated

  • Start Time: 2021-05-01T20:38:42.808+0000

  • Duration: 133 min 39 sec

  • Commit: a379684

Test stats 🧪

Test Results
Failed 0
Passed 47167
Skipped 5246
Total 52413

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 47167
Skipped 5246
Total 52413

@ph ph added the v7.13.0 label Apr 28, 2021
@ph
Copy link
Contributor

ph commented Apr 28, 2021

@urso you need to run make update in the elastic agent directory because you had updated the spec files.

@ph ph assigned urso Apr 28, 2021
@urso urso added backport-v7.13.0 Automated backport with mergify backport-v7.14.0 Automated backport with mergify labels Apr 28, 2021
@urso
Copy link
Author

urso commented Apr 28, 2021

/test

@urso
Copy link
Author

urso commented Apr 29, 2021

/test

"-E", "management.mode=x-pack-fleet",
"-E", "management.enabled=true",
"-E", "logging.level=debug",
"-E", "gc_percent=${METRICBEAT_GOGC:100}"
Copy link
Member

Choose a reason for hiding this comment

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

FB / MB are enough for now, but I assume we will need to apply it to every Golang process / beat we run?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, if we find this works well, we will extend on this and document the env vars.

runtime.GOMAXPROCS(maxProcs)
}
if gcPercent := b.Config.GCPercent; gcPercent > 0 {
logp.Info("Set gc percentage to: %v", gcPercent)
Copy link
Member

Choose a reason for hiding this comment

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

For the logs that are only logged once on startup (assuming this is correct) we should rather log too much then too little. Makes debugging easier.

@urso
Copy link
Author

urso commented Apr 30, 2021

/test

@mergify
Copy link
Contributor

mergify bot commented May 1, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b libbeat-set-gc-percentage upstream/libbeat-set-gc-percentage
git merge upstream/master
git push upstream libbeat-set-gc-percentage

@urso urso merged commit b21b87d into elastic:master May 2, 2021
@urso urso deleted the libbeat-set-gc-percentage branch May 2, 2021 11:47
mergify bot pushed a commit that referenced this pull request May 2, 2021
mergify bot pushed a commit that referenced this pull request May 2, 2021
(cherry picked from commit b21b87d)

# Conflicts:
#	x-pack/elastic-agent/pkg/agent/program/supported.go
urso pushed a commit that referenced this pull request May 3, 2021
Co-authored-by: Steffen Siering <steffen.siering@elastic.co>
urso pushed a commit that referenced this pull request May 3, 2021
Co-authored-by: Steffen Siering <steffen.siering@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.13.0 Automated backport with mergify backport-v7.14.0 Automated backport with mergify Team:Elastic-Agent Label for the Agent team v7.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants