[proposal] Remove interfaces and functions from go-elasticsearch.Config

[florianl]

Status Quo

go-elasticsearch.Config defines how one can interact with Elasticsearch. At the moment this struct holds a mix of types, functions and interfaces.

Functions:

go-elasticsearch/elasticsearch.go

Line 89 in 624594e

RetryOnError func(*http.Request, error) bool // Optional function allowing to indicate which error should be retried. Default: nil.

go-elasticsearch/elasticsearch.go

Line 103 in 624594e

RetryBackoff func(attempt int) time.Duration // Optional backoff duration. Default: nil.

go-elasticsearch/elasticsearch.go

Line 110 in 624594e

ConnectionPoolFunc func([]*elastictransport.Connection, elastictransport.Selector) elastictransport.ConnectionPool

Interfaces:

go-elasticsearch/elasticsearch.go

Lines 106 to 107 in 624594e

	Logger elastictransport.Logger // The logger object.
	Selector elastictransport.Selector // The selector object.

Problem

The functions and interfaces in go-elasticsearch.Config prevent users of elastic/go-elasticsearch to directly use go-elasticsearch.Config. The direct reuse of go-elasticsearch.Config is prevented, because it can not be configured, usually via YAML, and services are required to allow users to configure the connection to Elasticsearch. Common dependencies to handle configuration within the Elastic ecosystem are elastic/go-ucfg and elastic/elastic-agent-libs/config. As it is not possible to configure interfaces and/or functions with YAML, services that have to use go-elasticsearch.Config come up with their reimplemenation of go-elasticsearch.Config:

• elastic/apm-server
• elastic/fleet-server
• elastic/elastic-agent-shipper
• elastic/elastic-agent

None of these reimplementations of go-elasticsearch.Config fully cover all parts of go-elasticsearch.Config , lead to duplicate code and might introduce subtile bugs. In addition, configuring the retry functionality is different from service to service and prevents the reuse of configurations.

Proposal

Remove interfaces and functions from go-elasticsearch.Config. One option to keep functionality is, is to introduce Getter/Setter.

The suggested proposal is a breaking change and not backwards compatible.

[dmathieu]

How about:

Introduce a new StaticConfig struct within go-elasticsearch. This new struct doesn't include any functions and interfaces.
Then, use functional options in elasticsearch.New to allow building a new client with static config (from YAML), but also add other options such as RetryOnError.

type StaticConfig struct {
  Addresses []string `json:"addresses"`
}
myConfig := StaticConfig{}

client := elasticsearch.New(
  WithStaticConfig(myConfig),
)

With this, the user sets static config from the specified format.
I am specifying the JSON key in this struct here, as it's the easiest way to avoid folks from having to set their own config. But more complicated solutions using reflect may be possible if we want to be fancy.

If they want to specify function configs, which can't be specified from anything static, they can use other functional options:

client := elasticsearch.New(
  WithRetryOnError(func(*http.Request, error) bool {
    return false
  }),
)

Within the New method, a Config struct would be built, and every functional option would be passed to it. Each option decides what to do with the config (set one or multiple attributes).
If a specific functional option isn't passed, a default value has to be set.

[florianl]

What I'm missing from the suggested StaticConfig approach is a proper solution for RetryOnError and RetryBackoff. In particular these two interfaces provide currently configuration options, that are essential for customers.
If they are implemented with the functional approach, like WithRetryOnError(..) I can see how this works from a developer perspective. But I think, every service - like apm-server, fleet-server & others - will come up with their own way to configure it and so I think, the purpose to unify go-elasticsearch.Config accross the Elastic ecosystem is missed.