[tailsamplingprocessor] Support external decision cache implementations

[Logiraptor]

Description

Adding a feature. This PR adds support for external implementations of the decision cache. This allows the collector (or another service using the processor) to supply an alternative decision cache based on alternative algorithms or external services like memcached without needing to explicitly add support for all possible options in the processor.

It re-uses the existing function option pattern and only exposes two options for now: WithSampledDecisionCache and WithNonSampledDecisionCache. I've avoided exporting other options to avoid bloating the external interface without a concrete use case. The majority of changes are cleanup from the refactoring to move Option values into the Config struct instead of in a variadic parameter on newTracesProcessor.

[portertech]

@Logiraptor please add a changlog entry to .chloggen/.

[jpkrohling]

I'd prefer another approach to this, closer to what we have with the auth extensions, to where we plan to go with policies (#31582), and how we plan to support middlwares: via extensions.

Concretely, the config would look like this:

extensions:
    cacheimpl:
      url: ...
      max: 1_000

processors:
    tailsampling:
    cache: cacheimpl

To accomplish that, we'd need:

• the "cache extension" interface at extension/cache (perhaps even at core instead of contrib)
• one cache implementation (perhaps the current one we have can be copied over there)
• change the processor to load the proper extension. Here's how we do with auth:

https://github.com/open-telemetry/opentelemetry-collector/blob/ced38e8af2ae586363e3b2a34990e906a1227ccb/config/confighttp/confighttp.go#L199-L204

https://github.com/open-telemetry/opentelemetry-collector/blob/ced38e8af2ae586363e3b2a34990e906a1227ccb/config/configauth/configauth.go#L51-L58

[Logiraptor]

@jpkrohling Thanks for the feedback! I'm looking into that now, and I want to make sure I understand since it's my first time contributing to this repo. 🙏

So right now we have this interface:

// Cache is a cache using a pcommon.TraceID as the key and any generic type as the value.
type Cache[V any] interface {
	// Get returns the value for the given id, and a boolean to indicate whether the key was found.
	// If the key is not present, the zero value is returned.
	Get(id pcommon.TraceID) (V, bool)
	// Put sets the value for a given id
	Put(id pcommon.TraceID, v V)
	// Delete deletes the value for the given id
	Delete(id pcommon.TraceID)
}

When I create an extension, are you envisioning the same interface, or something more generic? I believe the key was not made generic intentionally to allow for optimizations in the implementation (using the right side of the id as a key).

If the cache should be more generic, then what about something that takes string keys and []byte values?

If the cache should not be more generic, then I'm wondering how useful it would be as an extension, since it presumable wouldn't be used by other components. I could be wrong, like I said this is my first contribution here so I'm still learning.

What do you think?

[Logiraptor]

I'll also note that even though the cache is generic on the value type, it's only ever used as Cache[bool].

[jpkrohling]

If the cache should not be more generic, then I'm wondering how useful it would be as an extension, since it presumable wouldn't be used by other components. I could be wrong, like I said this is my first contribution here so I'm still learning.

I think this is the case, and the name should therefore be something like TraceDecisionCache, or something like that. We can reuse it as Cache[string] in the load-balancing exporter to always route traces to the same backend even after a ring change.

[Logiraptor]

@jpkrohling Got it, ok so working through this I think there's another issue. I don't see how I can maintain type safety through the extension system while also supporting different value types. Specifically, the CreateFunc passed to extension.NewFactory must return a concrete type of something, and since it has no access to the downstream user of this extension, there's no way for it to know which type to return. (ie Cache[bool] or Cache[string])

Similarly, when the tsp tries to get the extension, it will have to type assert the value to some concrete type. So it seems these two pieces of code must implicitly agree on a single concrete type. In that case there's no benefit to using a type parameter. I think the only real solution here is either to forgo type safety using any or to use a type which is more generally useful like []byte. In the latter case, it's then up to each component using the cache to define some codec for converting to/from []byte. This seems to be what the storage extensions do, since they have signatures like

	Get(ctx context.Context, key string) ([]byte, error)
	Set(ctx context.Context, key string, value []byte) error

So now I'm thinking that this cache extension should be a very similar interface with different semantics. e.g. a cache does not guarantee durability between Get / Set whereas a storage does.

Thoughts?

[Logiraptor]

One way I've found which I think could work is like so

// CacheExtension is an extension that caches sampling decisions.
type CacheExtension interface {
	extension.Extension

	Cache[[]byte]
}

// Cache is a cache using a pcommon.TraceID as the key and any generic type as the value.
type Cache[V any] interface {
	// Get returns the value for the given id, and a boolean to indicate whether the key was found.
	// If the key is not present, the zero value is returned.
	Get(id pcommon.TraceID) (V, bool)
	// Put sets the value for a given id
	Put(id pcommon.TraceID, v V)
	// Delete deletes the value for the given id
	Delete(id pcommon.TraceID)
}

type Codec[V any] interface {
	// Encode encodes the value into a byte slice.
	Encode(v V) ([]byte, error)
	// Decode decodes the byte slice into a value.
	Decode(data []byte) (V, error)
}

func NewTypedCache[V any](codec Codec[V], cache Cache[[]byte]) (Cache[V])

Then the components interested in using the cache would essentially get a []byte cache from the system and then can safely convert it (with a codec) into a typed cache.

[Logiraptor]

Once that codec idea is in place, it would be trivial to add an "easy button" codec like json which can work for most types via reflection.

[Logiraptor]

@jpkrohling I went ahead and implemented that last idea to see how it looks: main...Logiraptor:opentelemetry-collector-contrib:logiraptor/decision-cache-extension

IMO it's OK but unfortunate that every use of the extension needs to pay an encoding cost even if only caching objects in memory. There will be a significant memory usage difference between the original implementation which is generic and this one which has to use []byte as the common denominator. Alternatively if the extension only works as a cache from traceID -> bool, then we wouldn't be able to reuse it in the load balancing exporter as you described.

What do you think I should do here?

[jpkrohling]

must return a concrete type of something, and since it has no access to the downstream user of this extension

I didn't see that coming :-/

I have a few ideas in mind, but I need some time to try it out (and it's been very, very hard to get any coding done this week on my side).

To unblock this, I think we can get back to the original approach, keeping in mind that we want to externalize this as an extension in the near future once we have a long-term plan. What do you think?

[Logiraptor]

@jpkrohling Sounds good to me. I will get the conflicts sorted out here then, and we can revisit once we have a solid plan.

[Logiraptor]

@jpkrohling Sorry for the delay, I have fixed the conflicts here and I think it's ready for a review.

[atoulme]

LGTM @jpkrohling ptal

[yvrhdn]

Hi 👋🏻 I'm helping @Logiraptor with this PR. He gave me permission to push to this PR.

I have updated the PR and merged the latest changes from main.

I have also added the tail sampling processor to checkapi/allowlist.txt: checkapi is a CI check that fails if there is a public function other than NewFactory. Since the goal of this PR is to allow configuring other cache implementations, it has to export WithSampledDecisionCache and WithNonSampledDecisionCache.
It's imo pretty much unavoidable that this check fails. I considered working around it, but it results in inconsistent codestyle.

[jpkrohling]

LGTM. This change is beneficial to downstream API consumers of the component, and I'd love to see a proposal that would make this configurable to our end-users as well.