Extending functional options

[pellared]

Why

It would be good to provide guidance on how to extend on top of functional options API.

What

Make vendor-specific (which may be not OTel compliant) extensibility "easy" and user-friendly.

The whole idea behind https://github.com/signalfx/splunk-otel-go is to create convenient and easy to use helpers that are working on top of OTel libraries. For example:

1. splunkhttp.NewHandler may set default otelhttp options
2. the user does not need to call otelhttp.NewHandler

I have experimented with 4 approaches. I will post them as separate comments so you can comment/upvote/downvote each one individually.

All of these approaches allow achieving the following usage code:

handler = splunkhttp.NewHandler(handler, "server", otelhttp.WithTracerProvider(provider), splunkhttp.WithTraceResponseHeader(false))

The splunkhttp.WithOTelOpts(....) call is no longer needed.

All of the approaches are not making any breaking change.

I posted this issue because I think we should strive to use similar extensibility approaches across different vendors.

Other remarks

Blocking extensibility by design?

Is there any reason to do such things? I would say it is blocking extensibility on purpose, but why? BTW even if I would like to do so I would rather unexport this method: https://github.com/open-telemetry/opentelemetry-go/blob/b09df84a24b35cb5f287a501e31d9dec324be5a9/trace/config.go#L41.

Answered here.

Why functional options cannot return an error?

One of the main advantages of functional options pattern is that it can be also used to encapsulate validation. How we would notify the client that his configuration passed to

opentelemetry-go-contrib/instrumentation/net/http/otelhttp/handler.go

Line 60 in e1a7c47

func NewHandler(handler http.Handler, operation string, opts ...Option) http.Handler {

is invalid?

Answered here.

[pellared]

Approach 1 - WithNop dummy option

Add WithNop functional option.

• Changes needed in OTel SDK: Add otelhttp.WithNop for easier extensibility #747
• Extending in vendor repo: https://github.com/pellared/splunk-otel-go/tree/poc-refacor-config-withnop/instrumentation/net/http/splunkhttp

It is not a breaking change - it does not touch any existing exported API.

Cons:

• Adding a dummy option to add extensibility feels wrong
• The extensibility code is noisy and not straightforward. Uses type embedding.

[pellared]

Approach 2 - Config

Export Config and unexport all its fields.

• Changes in OTel SDK: Export otelhttp.config and unexport its fields #748
• Extending the in vendor repo: https://github.com/pellared/splunk-otel-go/tree/poc-refacor-config-export-config/instrumentation/net/http/splunkhttp

It changes the existing API in a non-breaking way.

Cons:

• Having an exported struct with no exported fields feels wrong upfront
• The extensibility code is still noisy and not straightforward. Uses type embedding.

[pellared]

Approach 3 - interface{}

Change Apply in Option interface to accept interface{} instead of *config.

• Changes in OTel SDK: Refactor otelhttp.Option interface #749
• Extending the in vendor repo: https://github.com/pellared/splunk-otel-go/tree/poc-refacor-config-empty-interface/instrumentation/net/http/splunkhttp

It changes the existing API in a non-breaking way

Pros:

• The vendor's config.go code is similar ("orthogonal") to its OTel SDKs counterpart
• The extensibility code is the shortest and simplest than in other approaches

Cons:

• Forces to use type assertions in Apply method which is "less" type-safe (at least compiler is not helping that much) in OTel code.

I saw somewhere a variant of this approach where the factory function accepted opts ...interface{} instead of opts ...Option. However, it would be IMO worse because we would lose suggestions from intellisense.

From personal experience, I prefer type assertions over type embedding.

[pellared]

Approach 4 - type embedding + type assertion

Use type embedding and type assertion in vendor-specific repo.

• Changes in OTel SDK: NONE
• Extending the in vendor repo: https://github.com/pellared/splunk-otel-go/tree/refacor-config/instrumentation/net/http/splunkhttp

It does not require any change.

Pros:

• No changes needed in OTel SDK.

Cons:

• The extensibility code uses both type assertions and type embedding which is not straightforward. As a rule of thumb, both language features should be used in moderation.

[pellared]

I may be very biased and subjective. Feel free to point out other pros/cons and also feel free to argue if anything I described is not true.

I think that approach 3 is the cleanest solution. However approach 4 can be used to almost any functional option API.

Personally, I would go for approach 4.

However maybe after looking at this analysis we would still like to make some changes in the current OTel functional options design.

[tgulacsi]

otelhttp.Option is a function that applies to an inner structure with inner options - a black box.
I'd vote to option 4, as that's what respects this mostly.

Here you embed otelhttp's functionality, so you should embed their options, too - even better, hide them!
You don't want to allow ANY unknown option, as you don't know what to do with them.
Or if you want, then accept a preconfigured (otel)http.Handler, and decorate that only with splunk's functionality.

[pellared]

@tgulacsi First off all thanks for your comment 👍

You don't want to allow ANY unknown option, as you don't know what to do with them.

No sure if you are talking about Splunk or OTel package. However take notice that it is not possible to "not allow" external options (there is one possibility but it would result in very unfriendly API - unexport the Option interface). We can only make sure that external options are not processed. All described approaches achieve it (for OTel at least). Maybe for approach 2 it would be possible to change the config using reflection.

[tgulacsi]

otelhttp exposes several Option, but as a black box. Only those are usable os configuration-altering options.
At this level, exporting Option may be an error, but may help the documentation.

splunkhttp SHOULD NOT expose otelhttp options, MUST hide them behind a wrapper.

For me it's weird to allow any otelhttp option through splunkhttp. Esp. as they're in a constant flux...
It's more flexible to allow a configured http.Handler, that may be an otelhttp.Handler, already configured.

[MadVikingGod]

So a bit of logistics before we get into any technical discussion. It seems like you have done a lot of work to explore these options, but you need a bit more to make it presentable to this audience. What would go a long way to achieve this is:

1. An example of how you want to "Extend" the config. Not just how you are using it, but what feature would make most sense in your copy of the config and not upstream (in here)
2. Your changes in each proposal in a format that is normally used here. This would be a PR that encompasses just what was changed. Right now the PRs linked have all of the library in green, so it is hard to tell what is already there and what you changed.

Remember you are asking for a review from others that don't know what you know and have limited time to review. Try to make it easy for them to understand what you are changing and why.

As for the technical discussion:
Why are the configs not extendable? Because they are intended to be a black box, only used by the tool that the config exists within. Every change that we make to allow extensions has the potential to break extended versions as we change things. If we make config members public we can't ever move away from them. If we allow others to add members we will break any other implementation when we add a conflicting member. This doesn't mean it's a hard No on extensibility, but there has to be good reasons to take on that burden.

Returning errors:
It is totally possible to return errors there, but by not doing so we put a constraint on ourselves to not return a self-inconsistent handler. It may not work, but it won't work because of reasons that can not be determined at creation, e.g. wrong tls certs, not A and B can't be on at the same time.

[pellared]

@tgulacsi Thanks for clarification

otelhttp exposes several Option, but as a black box. Only those are usable os configuration-altering options.
At this level, exporting Option may be an error, but may help the documentation.

Returning an unexported type is highly discouraged. You cannot make a field, function, array variable (the last is critical for functional options) it the type is unexported. This is also why I have written "it would result in very unfriendly API". More: golang/lint#210

splunkhttp SHOULD NOT expose otelhttp options, MUST hide them behind a wrapper.

Thanks 👍 PRs fixed

For me it's weird to allow any otelhttp option through splunkhttp. Esp. as they're in a constant flux...
It's more flexible to allow a configured http.Handler, that may be an otelhttp.Handler, already configured.

Good remark. It is designed that way so that:

1. splunkhttp.NewHandler may set default otelhttp options
2. the user does not need to call otelhttp.NewHandler

The whole idea of https://github.com/signalfx/splunk-otel-go is to create convenient and easy to use helpers that are working on top of OTel libraries.

[pellared]

@MadVikingGod I highly appreciate your comment.

[...] What would go a long way to achieve this is: [...]

Great guidance 👍 Done. Please review and feel free to tell me if I can make it more approachable (without making too much noise).

Why are the configs not extendable?

Currently, I only think that approach 3 could make the extensibility easier. While still keeping the options as black boxes. But there is a drawback from OTel perspective (requires type assertion). Therefore, I guess approach 4 will be the favoured one.

Also based on your feedback I have created: #750

Returning errors

Not sure if I follow. But I believe it would be better to create a separate issue 😉

[Aneurysm9]

Good remark. It is designed that way so that:

1. splunkhttp.NewHandler may set default otelhttp options
2. the user does not need to call otelhttp.NewHandler

While I can understand the desire to have these properties, I think as it is currently structured it may prove overly limiting and lead to users to being unable to use it at all. By having splunkhttp.NewHandler embed the creation of an otelhttp.Handler it is limiting users to that instrumentation and either foreclosing users of other instrumentations such as otelecho or otelmux or forcing them to accept or find workarounds for double instrumentation.

If, instead, the splunkhttp handler were designed to be composed with another handler that was responsible for span lifecycle management, then the splunkhttp handler could deal with only the Options relevant to its internal workings and take the currently active span from the request.Context(). It could then be used in any of these ways:

splunkHandler := splunkhttp.NewHandler(handler, splunkhttp.WithFoo())

h := otelhttp.NewHandler(splunkHandler, otelhttp.WithBar())
h2 := otelmux.NewHandler(splunkHandler)
h3 := otelgin.NewHandler(splunkHandler, otelgin.WithBaz())

[pellared]

@Aneurysm9 Thanks for your insight 🙇

I think that what you presented can be done in Splunk in addition to one of the presented approaches. And it can be also reused internally. 👍

What you presented is more flexible and composable. However, it requires the client to write more code. Especially if we would like to set some default options (TBH I am not sure if there is a use case for it, but maybe someone might want to add some default filter?).

splunkHandler := splunkhttp.NewHandler(handler, splunkhttp.WithFoo())
opts := splunkhttp.DefaultHttpOpts()
opts = append(otelHttpOpts, otelhttp.WithBar())
instrHandler := otelhttp.NewHandler(splunkHandler, opts...)

Compare it to:

instrHandler := otelhttp.NewHandler(handler, splunkhttp.WithFoo(), otelhttp.WithBar())

Correct, the presented approach has to be implemented per each instrumentation. To achieve the same coding experience for otelmux and otelecho instrumentation, one would have to respectively create splunkmux and splunkecho. I just strive to offer ease of programming,

Besides I guess that your comment would also be valid for extending instrumentations done on top of database/sql package 😉

PS. nit: Your code snippett probably should have otelecho instead of otelgin 😉

[MadVikingGod]

If you are really looking to make programing this easier, then you should entirely encapsulate the otelhttp package. Users of your package shouldn't know or have to find out what otelhttp configuration options there are, they should be using splunkhttp options.

This lets you keep up with changes to otelhttp at your pace, and if you want to keep in lockstep I would look at code generation. It would also mean users have only one location they have to search to understand what knobs they can configure.

I would also point out in what you linked:

Orthogonality makes it easier to understand what happens when things combine.

If these are orthogonal features you should make the component orthogonal. But you look to want to encapsulate, so you should make your component encapsulate entirely the API.

[pellared]

Approach 5 - encapsulate the otelhttp and extend what is needed

by @MadVikingGod

Entirely encapsulate the otelhttp package and add all needed features.

• Changes in OTel SDK: NONE
• Extending the in vendor repo: use your imagination please 😉

It does not require any change.

Pros:

• The client does not need even to know nor import otelhttp
• No changes needed in OTel SDK.
• Possibily no dependency to otelhttp at all if the otelhttp code is copied instead of encapsulated

Cons:

• Requires synchronization if there is ANY change in otelhttp API which is an additional continuous maintenance cost.
• Lot of work to encapsulate/copy the otelhttp package. Could be solved by some kind of automation, but it is a more complex tool than both type embedding and type assertions.

[pellared]

@Aneurysm9

I am getting convinced that your proposal is the most pragmatic one (and probably optimal as well).
Besides what was already written there are more benefits of what you proposed:

1. Not mixing splunkhttp. and otelhttp. feels better and is easier to understand for the reader. @peterbourgon gave a nice rationale here
2. It does NOT force to use the default options if one would be provided by splunkhttp. Functional options often make "unseting" configuration very hard.
3. It is the simplest approach.

At the same time, I start to feel that each of my proposed approaches is overcomplicated compared to the benefits they bring (and the benefits are still very questionable).

[pellared]

Approach 6 - do NOT extend, but create another middleware

by @Aneurysm9

Create splunkhttp.NewHandler which may be composed with otelhttp.NewHandler

• Changes in OTel SDK: NONE
• Implementation in the vendor repo: https://github.com/pellared/splunk-otel-go/tree/refactor-splunkhttp/instrumentation/net/http/splunkhttp

Pros:

• The simplest, most composable and flexible approach
• Can be used for other instrumentation like otelmux and otelecho without writing any more code
• It is clearly distinguishable what is coming from OTel and what is not (more readable)
• No changes needed in OTel SDK

Cons:

• Client needs to write more code
• The client needs to make sure that the order of composing handlers is correct

[pellared]

@Aneurysm9 @MrAlias
We ended up with Approach 6.

Feel free to close the issue. I am not closing it, because maybe you want to have some follow up (e.g. docs) 😉 🤷