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

perf: set max conns idle per host as docappender max request #12035

Merged
merged 6 commits into from
Nov 16, 2023

Conversation

kruskall
Copy link
Member

Motivation/summary

By default the http transport will use 2 as the max idle connections per host.
The docappender client will send request a single host and will have at most
MaxRequest appenders sharing the same client. To improve performance and
connection reuse we set the number of max idle conns per host to MaxRequest

Checklist

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

Related issues

By default the http transport will use 2 as the max idle connections per host.
The docappender client will send request a single host and will have at most
MaxRequest appenders sharing the same client. To improve performance and
connection reuse we set the number of max idle conns per host to MaxRequest
sync default value with docappender default
@kruskall kruskall requested a review from a team as a code owner November 10, 2023 16:42
Copy link
Contributor

mergify bot commented Nov 10, 2023

This pull request does not have a backport label. Could you fix it @kruskall? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-7.17 is the label to automatically backport to the 7.17 branch.
  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Nov 10, 2023
@@ -700,6 +700,7 @@ func (s *Runner) newFinalBatchProcessor(
}
esConfig.FlushInterval = time.Second
esConfig.Config = elasticsearch.DefaultConfig()
esConfig.MaxIdleConnsPerHost = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Why setting it to 10 here and then only later to esConfig.MaxRequests?

Copy link
Member Author

@kruskall kruskall Nov 13, 2023

Choose a reason for hiding this comment

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

10 is the default value

We overwrite this value with esConfig.MaxRequests after the config has been unpacked.

I've made it more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other default values are set in the elasticsearch/config.go; IMO the default value should also be set there.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would change the value for every ES client which might not be ideal.

The default value of 10 only makes sense for the docappender client because it's the same as the default number of max requests handled by the docappender.

Copy link
Contributor

Choose a reason for hiding this comment

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

So in this case, shouldn't this default value then always be set via esConfig.MaxRequests in line 724? Why setting it to a hardcoded default, that easily gets outdated (as the docappender default value comes from a different source)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The hardcoded default is for when MaxRequests is 0.

Without this line, MaxIdleConnsPerHost will use a default value of 0 and the http.Transport will use the internal default (which is 2).

The docappender on the other hand will use a default value of 10, leading to the same issue we have right now (resources being underused and connections being recreated).

We need to pass the es config before creating the es client which is then passed to the docappender so we can't do this in the go-docappender library.

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

LGTM; would be good to have some actual numbers on this improvement.

@kruskall kruskall enabled auto-merge (squash) November 16, 2023 08:31
@kruskall kruskall merged commit 763ed5f into elastic:main Nov 16, 2023
7 checks passed
@kruskall kruskall deleted the perf/idle-max-request branch November 16, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants