Introduce overrides.Interface to decouple implementation from usage

[yvrhdn]

What this PR does:

A lot of modules in Tempo rely on the overrides package for runtime config, they directly use the overrides.Overrides struct. This makes the coupling very tight and limits modularity.

Changes:

• extract overrides.Interface from the methods of overrides.Overrides
• use this interface in as much places as possible
• export the Overrides module from App

Which issue(s) this PR fixes:
Fixes #

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[yvrhdn]

This method wasn't used, I've removed it and added a type assertion to avoid drift.

[joe-elliott]

this is a really intense interface which will make implementation/mocking difficult. is there anything we can do to simplify?

maybe:

type Interface interface {
  Limit(type, userID) 
}

That doesn't seem to work b/c each limit returns a different type and returning empty interface is kind of gross.

Is there a better point of abstraction? maybe something inside the overrides module?

[yvrhdn]

this is a really intense interface which will make implementation/mocking difficult. is there anything we can do to simplify?

It is very intense yeah, this PR isn't really intended as a refactor. It's just extracting the interface making the contract between overrides and the rest of Tempo more explicit. AFAIK it shouldn't make testing more difficult than it already is.

Regarding testing:

A pattern I adopted in other modules is to create a smaller 'localized' overrides interface. For example the metrics-generator registry has an overrides interface with just 3 overrides:

tempo/modules/generator/registry/overrides.go

Lines 9 to 15 in 68ed18c

	type Overrides interface {
	MetricsGeneratorMaxActiveSeries(userID string) uint32
	MetricsGeneratorCollectionInterval(userID string) time.Duration
	MetricsGeneratorDisableCollection(userID string) bool
	}
	var _ Overrides = (*overrides.Overrides)(nil)

The registry only depends on the smaller interface and it's easy to mock that. With type assertion we can ensure it still matches with the bigger overrides.Interface.

In a lot of test we currently use overrides.NewOverrides(overrides.Limits{...}) and that will still work. Example:

tempo/modules/compactor/compactor_test.go

Lines 18 to 20 in 68ed18c

	o, err := overrides.NewOverrides(overrides.Limits{
	MaxBytesPerTrace: math.MaxInt,
	})

Regarding simplifying/refactoring:

I can't really think of a quick way to simplify this without changing a lot of code across the entire codebase. If we refactor overrides entirely we could end up with something like this:

type Overrides interface {
  Limit(type, userID string) (value interface{}, isSet boolean)
}

We currently can't differentiate between 0 and unset limits so there is definitely some gain from refactoring this, it isn't really the scope of this PR though 😅

[joe-elliott]

Yup, these are all good points. One final question. What about building an interface around the runtimeconfig.Manager:

https://github.com/grafana/tempo/blob/main/modules/overrides/overrides.go#LL72C34-L72C41

Currently this is hard coded to pull from a file, but if we make this our point of abstraction we could pull from an HTTP endpoint, GCS bucket, whatever. Does this meet your needs?

Also, we could presumably PR these changes upstream if dskit wants them.

[yvrhdn]

Interesting, I think extending/reusing code of the runtimeconfig.Manager to work with other sources would be neat. The polling logic will be needed for every source I think.

Besides fetching the limits we also need logic to merge the overrides of multiple sources and I think that will still end up somewhere in Tempo and not in dskit since dskit isn't aware of our limits. Adding a first layer of abstraction (the overrides.Interface) will make it easier to put a component in between that can deal with the merging logic.

[joe-elliott]

understood 👍