Unify Configuration and Initialization

[MrAlias]

Being consistent in how we configure and create things in the project means users can easily anticipate what they need to do. Currently this is not the case.

We should standardize on the approach outlined in this comment:

type Option func(*Config)
type Config struct { ... }
func NewT(config Config) *T { ... }
func ConfigureT(opts ...Option) *T { 
  config := Config{}
  for _, opt := range opts {
    opt(&config)
  }
  t := NewT(config)
  // ...
}

[Aneurysm9]

There was discussion regarding whether Option should be a function or an interface. I think there may be situations where an Option might require more than a bare function, so I would propose the following types:

type Option interface {
  Apply(*Config)
}

type OptionFunc func(*Config)

func (o OptionFunc) Apply(c *Config) {
  o(c)
}

Then simple options can be implemented in functions and wrapped with OptionFunc:

func WithName(name string) Option {
  return OptionFunc(func(c *Config) {
    c.name = name
  })
}

[krnowak]

One place where bare functions weren't enough was in metrics options for instruments. We used to have three kinds of instruments (counters, gauges and measures). They had some common options (like WithUnit or WithDescription). They also had their own specific options (like measure had WithAbsolute). Some of them shared some option name (like gauge and counter had WithMonotonic option). This is all gone and I'm not sure if they'll come back, but anyway. To solve I had:

type Config struct {…}
// measure-specific options
type MeasureOption interface {
    ApplyMeasure(*Config)
}
// gauge-specific options
type GaugeOption interface {…}
// counter-specific options
type CounterOption interface {…}
// general option (with unit, with description)
// this can be passed as an instrument-specific option too
type Option interface {
    MeasureOption
    GaugeOption
    CounterOption
}

func WithDescription(n string) Option {…}
func WithAbsolute(a bool) MeasureOption {…}

func NewMeasure(n string, o ...MeasureOption) Measure {…}
func NewCounter(n string, o ...CounterOption) Counter {…}

so NewMeasure("foo", WithUnit("cm"), WithAbsolute(true)) worked fine, but NewCounter("bar", WithAbsolute(false)) was a compilation error as desired.

So I'd amend the proposal above to what @Aneurysm9 proposed:

type Option interface {
  // this can be named differently in complex cases
  Apply(*Config)
}
// this can be a struct in complex cases
type OptionFunc func(*Config)
func (o OptionFunc) Apply(c *Config) {
  o(c)
}
type Config struct { ... }
// Not sure to standardize on those, though
func Configure(opts ...Option) Config {
  config Config{}
  for _, opt := range opts {
    opt(&config)
  }
  return config
}
func NewT(config Config) *T { ... }
func ConfigureT(opts ...Option) *T { 
  t := NewT(Configure(opts…))
  // ...
}

[johananl]

Happy to take this one @MrAlias.

[krnowak]

Maybe one thing that needs to be looked at, is how to configure two same things within one object. So an example would be in otlp exporter, where we can configure (not in master yet) two connections separately (one for sending metrics, one for sending traces).

So I don't have an ideal solution yet, but that's what I have done for now:

type Config struct {
	// unexported fields
}

type Option func(*Config)

// makes a copy of config
func (Config) Apply(Option ...opts) Config { … }

// modifies config in place
func (*Config) ApplyInPlace(Option ...opts) { … }

type ConnectionManager interface { … }

func NewSingleConnectionManager(cfg Config)  ConnectionManager { … }

type MultiConnectionConfig struct {
	Tracing Config
	Metrics Config
}

func NewMultiConnectionManager(cfg MultiConnectionConfig) ConnectionManager { … }

so the use is:

cfg := Config{}.Apply(WithThis(…), WithThat(…))
cm := NewSingleConnectionManager(cfg)
// or
cfg := MultiConnectionConfig{}
cfg.Tracing.ApplyInPlace(WithThis(…), WithThat(…))
cfg.Metrics.ApplyInPlace(WithThis(…), WithSomethingElse(…))
cm := NewMultiConnectionManager(cfg)

For NewMultiConnectionManager, having func NewMultiConnectionManager(opts ...Option) ConnectionManager would not work, because how do we configure tracing connection separately from metrics connection without doing WithTracingThis and WithMetricsThis?

[johananl]

My WIP is here: https://github.com/johananl/opentelemetry-go/tree/johananl/refactor-config-init

[MrAlias]

Documented ref: https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#configuration

[pellared]

Just to make sure what is left to be done:

1. Double-check documented ref

A little change proposal (by example, NOT in docs) can be found here: open-telemetry/opentelemetry-go-contrib#750

2. Apply the pattern in the whole OTel Go codebase

Probably a PR per config type would be the most effective approach.

---

Please confirm.

I plan to put my 👀 on this issue on Monday.

Side note: This might be also seen as a related issue: open-telemetry/opentelemetry-go-contrib#746

[MrAlias]

@pellared I think we're in agreement on the overall strategy. I would add that we should update the docs before we update the codebase. I like the idea presented in open-telemetry/opentelemetry-go-contrib#750. We should updated the docs to match and proceed as you outlined.

[pellared]

@MrAlias PR for the docs: #1841

[MrAlias]

Question: should this be done in multiple PRs?
Answer: We probably want to do this in one PR to make sure the solution is applied consistently.

Question: should we start in contrib or here?
Answer: here

[MadVikingGod]

There is one deviation we must make. The API defines a number of configs trace.TracerConfig and trace.SpanConfig for example. Because these configs are intended to be consumed by the SDK and not internal to the package, they must be exported.

I think it is still prudent to have the struct members unexported, configured via Options, and accessed via methods. Is there any reason why the Options should be implementable by other packages?

[pellared]

There is one deviation we must make. The API defines a number of configs trace.TracerConfig and trace.SpanConfig for example. Because these configs are intended to be consumed by the SDK and not internal to the package, they must be exported.

I think it is still prudent to have the struct members unexported, configured via Options, and accessed via methods. Is there any reason why the Options should be implementable by other packages?

Initially I wanted to implement it in the exact way you proposed. Unexport the fields and add exported getters.

However I was not sure if this is the way to go. I would need more time to analyze how SDK uses it and experiment with some alternative design.