[RFC] Functional Interface pattern for Collector extension APIs

[jmacd]

trafficstars

Description

Replaces https://github.com/open-telemetry/opentelemetry-collector/pull/13263

Documents the interface pattern used in core interfaces (e.g., component, consumer, extensions), which enables safe interface evolution as described in this RFC. This documents an existing pattern so that it can be applied more consistently across our code base.

Testing Documentation

This interface pattern has been checked and documented using examples from PR https://github.com/open-telemetry/opentelemetry-collector/pull/13241.

[jmacd]

I've referred to my former attempt at this many times in the months since I first opened it, so here it is again. Following feedback from @evan-bradley I changed the name to "Functional Interface" pattern. Any name will do, really.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 92.21%. Comparing base (c08dbf5) to head (936a777).

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13902      +/-   ##
==========================================
- Coverage   92.22%   92.21%   -0.02%     
==========================================
  Files         658      658              
  Lines       41168    41168              
==========================================
- Hits        37968    37963       -5     
- Misses       2190     2193       +3     
- Partials     1010     1012       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jmacd]

https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/42573 is an example of an ad-hoc extension mechanism in collector-contrib: it might be valuable to apply these patterns in that repository.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jade-guiton-dd]

This is a bit difficult to review comprehensively, but here is some initial feedback from reading it:

1. Who do these "MUST" requirements apply to? Is it just the core Collector repository? Should we maybe restrict this to non-experimental public interfaces which are part of the Core Collector API and meant to be wildly implemented?
2. How sure are we that our existing codebase fulfills these requirements? Are we sure we don't have public interfaces somewhere that don't use this pattern?
3. I'm not convinced that there is a pressing need to enforce this pattern strictly, although documenting it is always good. One potential limitation I can think of is that, if I'm not mistaken, it's impossible to have a struct implement two such interfaces, correct?
4. Is there a need to require defining No-op behavior for required methods? (ie. ones defined as positional arguments in the constructor instead of options)
5. I think that the presentation could be simplified and shortened, as I find this to be a somewhat tough read. (Perhaps that's part of the reason reviewers have been shy thus far). Here are some ideas:
   1. I think there are too many examples. I think a single one showing everything at once would be simpler to understand.
   2. It's especially confusing that some of the current examples are only "partial" implementations. For example, the phrase "Still, an interface is used so that it can be sealed, as in:" precedes an example which doesn't implement sealing.
   3. This is a sufficiently abstract pattern that I think it would be best to lead with an example, and then explain it using the "key concepts", instead of leading with an abstract description of it.
   4. I would also simplify the example to be more abstract (like having Foo methods), to avoid having to understand what a "rate limiter with a non-block reservation" is to understand the pattern.
   5. To make the document more actionable, I would also recommend that the example show a "before and after": show the "naive" way an interface would be defined and implemented by a struct, then show how it's done with this pattern.
   6. I think the terms "Extension types" and "Performance interfaces" could be made clearer. ("Extension interfaces" and "Performance-sensitive interfaces" maybe?)
   7. Does it make sense to call this "the Functional Interface Pattern"? It seems to wrap multiple ideas into one (sealed interfaces with a single canonical implementation, defining named types for each method with a "nil" implementation, the option pattern to define optional fields...). Maybe we could call it the "Partial Interface Pattern" or something like that?

[jmacd]

Thank you @jade-guiton-dd. Having received some feedback, I will return this to draft and work on improving it.

I do want to address the central questions, and why I think this is important.

Who do these "MUST" requirements apply to? Is it just the core Collector repository? Should we maybe restrict this to non-experimental public interfaces which are part of the Core Collector API and meant to be wildly implemented?

This is not about all interfaces, this is about extension interfaces, which includes some of the core interfaces but not all of them. For example, component.Config is not an extension interface. (Extension interfaces can't be empty.)

I come to this because I worked to add middleware and ratelimiter extension support, and in the code reviews there were many undocumented requirements stated, in particular by @bogdandrutu. I had to rewrite these components several times because at first, the requirements were not clear, and they are still not clear hence this RFC.

How sure are we that our existing codebase fulfills these requirements? Are we sure we don't have public interfaces somewhere that don't use this pattern?

I am sure the existing codebase does not fulfil these requirements. It is self-inconsistent and I want to fix that situation; the existing self-inconsistency is part of the undocumented requirement problem--requirements are being asked that the codebase does not meet itself. The existing self-inconsistency is the reason why extension interfaces are hard to add.

I'm not convinced that there is a pressing need to enforce this pattern strictly, although documenting it is always good. One potential limitation I can think of is that, if I'm not mistaken, it's impossible to have a struct implement two such interfaces, correct?

No, nothing prevents a component from implementing multiple extension interfaces, except the presence of method-name re-use. We avoided extensionauth.Client.RoundTripper being the same as extensionmiddleware.Client.GetRoundTripper for this reason, even though they are the same method. Again, when I put rate-limiter changes up for review, these requirements felt very difficult to meet, being unstated.

Is there a need to require defining No-op behavior for required methods? (ie. ones defined as positional arguments in the constructor instead of options)

Again, this requirement was given as I worked on middleware extensions. I would say that extensions are currently impossible to contribute to the code base, because of undocumented requirements. I will try to improve the document in the ways you suggested.

[jade-guiton-dd]

This is not about all interfaces, this is about extension interfaces, which includes some of the core interfaces but not all of them.

I see, that makes sense to me. You may want to update the document to make that clearer, as right now it sounds like this would apply to all "major interfaces".

I wasn't aware of these being existing (but unwritten) requirements for extension interfaces, and would not have known to enforce them. Sounds like it's a great idea to write them down and make sure all maintainers agree on them to avoid arbitrary enforcement and inconsistencies, so thank you for taking it on.

No, nothing prevents a component from implementing multiple extension interfaces.

Hmm, would it be something like this?

type myExtension struct {
	CoolInterface1
	CoolInterface1
}
func createExtension(ctx context.Context, set extension.Settings, cfg component.Config) (extension.Extension, error) {
	ext := &myExtension{}
	ext.CoolInterface1 = NewCoolInterface1(Method1Func(ext.myMethod1))
	ext.CoolInterface2 = NewCoolInterface2(Method2Func(ext.myMethod2))
	return ext
}
func NewFactory() extension.Factory {
	return extension.NewFactory(metadata.Type, createDefaultConfig, createExtension, metadata.ExtensionStability)
}

I don't think the current version has an example of how to implement the interface externally, I think that could be a useful addition (although not as useful for developers of new interfaces, which seems like the primary audience).