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

Sets gomaxprocs in core agent before components initialize #25021

Merged
merged 2 commits into from
Apr 23, 2024

Conversation

scottopell
Copy link
Contributor

What does this PR do?

This reorders the core agent startup so that the custom logic we have for setting GOMAXPROCS will always finish before any other component initializes.

Motivation

Components can and do (see dogstatsd) use the value of GOMAXPROCS to make decisions about how many of a certain component to run.

If the GOMAXPROCS env var is set using a millicore suffix as is supported by the Agent, this currently races against component initialization (and loses in practice).

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

QA Done locally, confirmed that GOMAXPROCS is set correctly and core agent runs properly.

@scottopell scottopell added changelog/no-changelog team/single-machine-performance Single Machine Performance qa/done QA done before merge and regressions are covered by tests labels Apr 23, 2024
@scottopell scottopell added this to the 7.54.0 milestone Apr 23, 2024
@scottopell scottopell requested review from a team as code owners April 23, 2024 15:18
pkg/aggregator/demultiplexer.go Outdated Show resolved Hide resolved
@scottopell
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Apr 23, 2024

🚂 MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

@pr-commenter
Copy link

pr-commenter bot commented Apr 23, 2024

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=32733047 --os-family=ubuntu

@dd-devflow
Copy link

dd-devflow bot commented Apr 23, 2024

🚂 MergeQueue

Pull request added to the queue.

This build is next! (estimated merge in less than 49m)

Use /merge -c to cancel this operation!

@dd-mergequeue dd-mergequeue bot merged commit 57b1bf9 into main Apr 23, 2024
192 of 193 checks passed
@dd-mergequeue dd-mergequeue bot deleted the sopell/set-gomaxprocs-correctly branch April 23, 2024 20:26
@@ -307,6 +304,15 @@ func run(log log.Component,

func getSharedFxOption() fx.Option {
return fx.Options(
fx.Invoke(func(lc fx.Lifecycle) {
Copy link
Contributor

@ogaca-dd ogaca-dd Apr 26, 2024

Choose a reason for hiding this comment

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

ddruntime.SetMaxProcs() is called after every component constructor and after OnStart hooks. I guess this function should be called as earlier as possible. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, I didn't realize that this won't run before other component's constructors, which would be desirable (although not necessary for the specific bug I'm working around here).

If you can offer a suggested way to run this as early as possible, but still have access to logger, I'd be happy to submit a follow-up to improve this.

Copy link
Contributor

Choose a reason for hiding this comment

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

One option is to replace

return fxutil.OneShot(run,
			fx.Supply(core.BundleParams{

by

return fxutil.OneShot(run,
			fx.Invoke(func(_ log.Component) {
				ddruntime.SetMaxProcs()				
			}),
			fx.Supply(core.BundleParams{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I tested this and it works as expected. Available for review in #25198

alexgallotta pushed a commit that referenced this pull request May 9, 2024
* Sets gomaxprocs before other components initialize and logs out dsd worker calculation

* Update pkg/aggregator/demultiplexer.go

Co-authored-by: Vickenty Fesunov <[email protected]>

---------

Co-authored-by: Vickenty Fesunov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog qa/done QA done before merge and regressions are covered by tests team/single-machine-performance Single Machine Performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants