-
Notifications
You must be signed in to change notification settings - Fork 79
Change RemoteEndpoints interface to support remote engines pruning
#680
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
63ff685
ba2303e
b51950a
75d41f3
808c854
8840588
e975db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ package api | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "math" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/prometheus/prometheus/model/labels" | ||
|
|
@@ -16,10 +18,46 @@ type RemoteQuery interface { | |
| fmt.Stringer | ||
| } | ||
|
|
||
| // Deprecated: RemoteEndpoints will be replaced with | ||
| // RemoteEndpointsV2 / RemoteEndpointsV3 in a future breaking change. | ||
| type RemoteEndpoints interface { | ||
| Engines() []RemoteEngine | ||
| } | ||
|
|
||
| // RemoteEndpointsV2 describes endpoints that accept pruning hints when | ||
| // selecting remote engines. | ||
| // | ||
| // For example implementations may use the hints to prune the TSDBInfos, but | ||
| // also may safely ignore them and return all available remote engines. | ||
| // | ||
| // NOTE(Aleksandr Krivoshchekov): | ||
| // We add a new interface as a temporary backward compatibility. | ||
| // RemoteEndpoints will be replaced with it in a future breaking change. | ||
| type RemoteEndpointsV2 interface { | ||
| EnginesV2(mint, maxt int64) []RemoteEngine | ||
| } | ||
|
|
||
| type RemoteEndpointsQuery struct { | ||
| MinT int64 | ||
| MaxT int64 | ||
| } | ||
|
|
||
| // RemoteEndpointsV3 describes endpoints that accept pruning hints when | ||
| // selecting remote engines. | ||
| // | ||
| // For example implementations may use the hints to prune the TSDBInfos, but | ||
| // also may safely ignore them and return all available remote engines. | ||
| // | ||
| // NOTE(Aleksandr Krivoshchekov): | ||
| // We add a new interface as a temporary backward compatibility. | ||
| // RemoteEndpoints will be replaced with it in a future breaking change. | ||
| // | ||
| // Unlike RemoteEndpointsV2, this interface can be extended with more hints | ||
| // in the future, without making any breaking changes. | ||
| type RemoteEndpointsV3 interface { | ||
| EnginesV3(query RemoteEndpointsQuery) []RemoteEngine | ||
| } | ||
|
|
||
| type RemoteEngine interface { | ||
| MaxT() int64 | ||
| MinT() int64 | ||
|
|
@@ -44,6 +82,71 @@ func (m staticEndpoints) Engines() []RemoteEngine { | |
| return m.engines | ||
| } | ||
|
|
||
| func (m staticEndpoints) EnginesV2(mint, maxt int64) []RemoteEngine { | ||
| return m.engines | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we apply filtering here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried, but since pruning is optional, I decided to not change Some some unit tests check for that, and if we apply filter in static endpoints, they fail. But now when I'm thinking about that again, I'm not sure that's an issue anymore.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think yes, the API contract suggest that we only should return intersecting engines.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you suggesting something like this? func (m staticEndpoints) Engines(mint, maxt int64) []RemoteEngine {
var engines []RemoteEngine
for _, e := range m.engines {
if e.MaxT() < mint || e.MinT() > maxt {
continue
}
engines = append(engines, e)
}
return engines
}
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If so, I'm a bit hesitant about filtering engines themselves here (Maybe I don't fully understand the original implementation). The optimizers seem to expect all engines to be available, especially for handling Also, filtering engines breaks --- Expected
+++ Actual
@@ -1 +1 @@
-sum(sum by (region) (metric @ 18000.000))
+sum(dedup(remote(sum by (region) (metric @ 18000.000))))Let me know if I'm mistaken, but it seems like an invalid result. The current "real" implementation also assumes all engines are return. The intent of the Does that make sense, or am I missing something?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha, this indeed makes sense, especially since you brought up |
||
| } | ||
|
|
||
| func (m staticEndpoints) EnginesV3(query RemoteEndpointsQuery) []RemoteEngine { | ||
| return m.engines | ||
| } | ||
|
|
||
| func NewStaticEndpoints(engines []RemoteEngine) RemoteEndpoints { | ||
| return &staticEndpoints{engines: engines} | ||
| } | ||
|
|
||
| type cachedEndpoints struct { | ||
| endpoints RemoteEndpoints | ||
|
|
||
| enginesOnce sync.Once | ||
| engines []RemoteEngine | ||
| } | ||
|
|
||
| func (l *cachedEndpoints) Engines() []RemoteEngine { | ||
| return l.EnginesV3(RemoteEndpointsQuery{ | ||
| MaxT: math.MinInt64, | ||
| MinT: math.MaxInt64, | ||
| }) | ||
| } | ||
|
|
||
| func (l *cachedEndpoints) EnginesV2(mint, maxt int64) []RemoteEngine { | ||
| return l.EnginesV3(RemoteEndpointsQuery{ | ||
| MaxT: maxt, | ||
| MinT: mint, | ||
| }) | ||
| } | ||
|
|
||
| func (l *cachedEndpoints) EnginesV3(query RemoteEndpointsQuery) []RemoteEngine { | ||
| l.enginesOnce.Do(func() { | ||
| l.engines = getEngines(l.endpoints, query) | ||
| }) | ||
| return l.engines | ||
| } | ||
|
|
||
| func getEngines(endpoints RemoteEndpoints, query RemoteEndpointsQuery) []RemoteEngine { | ||
| if v3, ok := endpoints.(RemoteEndpointsV3); ok { | ||
| return v3.EnginesV3(query) | ||
| } | ||
|
|
||
| if v2, ok := endpoints.(RemoteEndpointsV2); ok { | ||
| return v2.EnginesV2(query.MinT, query.MaxT) | ||
| } | ||
|
|
||
| return endpoints.Engines() | ||
| } | ||
|
|
||
| // NewCachedEndpoints returns an endpoints wrapper that | ||
| // resolves and caches engines on first access. | ||
| // | ||
| // All subsequent Engines calls return cached engines, ignoring any query | ||
| // parameters. | ||
| func NewCachedEndpoints(endpoints RemoteEndpoints) RemoteEndpoints { | ||
| if endpoints == nil { | ||
| panic("api.NewCachedEndpoints: endpoints is nil") | ||
| } | ||
|
|
||
| if le, ok := endpoints.(*cachedEndpoints); ok { | ||
| return le | ||
| } | ||
|
|
||
| return &cachedEndpoints{endpoints: endpoints} | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.