[PoC] Refactor config API #33
Conversation
|
I tried 3 approaches to make OTel functional options API more extensible-friendly.
Approach 1I do not like adding the Approach 2It does not have the drawback of approach 1, however, it will result in a similar code boat to opinion 2, It requires type-assertions (but not in OTel code - just in "distro" code). When I started writing this code I ended with approach no 3. Approach 3I feel that this one is the cleanest. We lose some kind of type-safety as we really on type assertions, but I cannot figure out any better way that will work nicely with the functional options approach. The implementation of options in OTEL and distro is similar (orthogonal). I was also thinking of builder pattern or simply using config structs... But after a few hours of experiments, I just feel that I am not able to come up with anything better than presented in this PR. |
| @@ -2,6 +2,8 @@ module github.com/signalfx/splunk-otel-go/instrumentation/net/http/splunkhttp | |||
|
|
|||
| go 1.14 | |||
|
|
|||
| replace go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp => ../otelhttp | |||
There was a problem hiding this comment.
I have "vendored" go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp module to easily experiment and visualize the changes in OTel repo
| // Option Interface used for setting *optional* config properties | ||
| type Option interface { | ||
| Apply(interface{}) | ||
| } | ||
|
|
||
| // OptionFunc provides a convenience wrapper for simple Options | ||
| // that can be represented as functions. | ||
| type OptionFunc func(*config) | ||
|
|
||
| func (o OptionFunc) Apply(obj interface{}) { | ||
| if c, ok := obj.(*config); ok { | ||
| o(c) | ||
| } | ||
| } |
There was a problem hiding this comment.
these are the only changes required in OTel repo
this is the current state for comparison:
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/5b82c086071c7d152c3ea72fe1f3a455254acfc5/instrumentation/net/http/otelhttp/config.go#L48-L59
|
Awesome work! 😄 I like option 2 the most due to the type safety, but I'm still thinking it through. I definitely want to present this to the Go SIG on Thursday to get more feedback. |
I will try to make a draft PR for each approach + GitHub issue in opentelemetry-go-contrib to facilitate the discussion. |
Why
#30 (comment)
What
See: 2c907eb and d8365c1
It is an attempt to make the OTel functional options API more extensible-friendly.