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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b13b9d
perf: set max conns idle per host as docappender max request
kruskall Nov 10, 2023
3f0ccbc
fix: use default value of 10
kruskall Nov 10, 2023
e7c4819
fix: only overwrite maxidleconnsperhost if maxrequests is not zero
kruskall Nov 13, 2023
20f07a8
fix: set max idle conns per host before creating the es client
kruskall Nov 13, 2023
bd127ae
Merge branch 'main' into perf/idle-max-request
kruskall Nov 13, 2023
6d23501
Merge branch 'main' into perf/idle-max-request
simitt Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/beater/beater.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,10 +700,15 @@ 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 •
edited
Loading

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.

if err := s.elasticsearchOutputConfig.Unpack(&esConfig); err != nil {
return nil, nil, err
}

if esConfig.MaxRequests != 0 {
esConfig.MaxIdleConnsPerHost = esConfig.MaxRequests
}

var flushBytes int
if esConfig.FlushBytes != "" {
b, err := humanize.ParseBytes(esConfig.FlushBytes)
Expand Down
34 changes: 18 additions & 16 deletions internal/elasticsearch/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,19 @@ var (

// Config holds all configurable fields that are used to create a Client
type Config struct {
Hosts Hosts `config:"hosts" validate:"required"`
Protocol string `config:"protocol"`
Path string `config:"path"`
ProxyURL string `config:"proxy_url"`
ProxyDisable bool `config:"proxy_disable"`
Timeout time.Duration `config:"timeout"`
TLS *tlscommon.Config `config:"ssl"`
Username string `config:"username"`
Password string `config:"password"`
APIKey string `config:"api_key"`
Headers map[string]string `config:"headers"`
MaxRetries int `config:"max_retries"`
Hosts Hosts `config:"hosts" validate:"required"`
Protocol string `config:"protocol"`
Path string `config:"path"`
ProxyURL string `config:"proxy_url"`
ProxyDisable bool `config:"proxy_disable"`
Timeout time.Duration `config:"timeout"`
TLS *tlscommon.Config `config:"ssl"`
Username string `config:"username"`
Password string `config:"password"`
APIKey string `config:"api_key"`
Headers map[string]string `config:"headers"`
MaxRetries int `config:"max_retries"`
MaxIdleConnsPerHost int `config:",ignore"`

// CompressionLevel holds the gzip compression level used when bulk indexing
// with go-docappender; it is otherwise ignored.
Expand Down Expand Up @@ -145,9 +146,10 @@ func NewHTTPTransport(cfg *Config) (*http.Transport, error) {
dialer := transport.NetDialer(cfg.Timeout)
tlsDialer := transport.TLSDialer(dialer, tlsConfig, cfg.Timeout)
return &http.Transport{
Proxy: proxy,
Dial: dialer.Dial,
DialTLS: tlsDialer.Dial,
TLSClientConfig: tlsConfig.ToConfig(),
Proxy: proxy,
Dial: dialer.Dial,
DialTLS: tlsDialer.Dial,
TLSClientConfig: tlsConfig.ToConfig(),
MaxIdleConnsPerHost: cfg.MaxIdleConnsPerHost,
}, nil
}
9 changes: 5 additions & 4 deletions internal/elasticsearch/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,10 +146,11 @@ func TestBeatsConfigSynced(t *testing.T) {
// TODO(simitt): take a closer look at ES ouput changes in libbeat
// introduced with https://github.com/elastic/beats/pull/25219
localStructExceptions := map[string]interface{}{
"ssl": nil,
"timeout": nil,
"proxy_disable": nil,
"proxy_url": nil,
"ssl": nil,
"timeout": nil,
"proxy_disable": nil,
"proxy_url": nil,
"maxidleconnsperhost": nil,
}
for name, localStructField := range localStructFields {
if _, ok := localStructExceptions[name]; ok {
Expand Down