Update mimir-prometheus to 320f0c9c4a88 #6085
Conversation
aknuds1
force-pushed
the
arve/revendor-prometheus
branch
4 times, most recently
from
984e312
to
3621699
Compare
aknuds1
force-pushed
the
arve/revendor-prometheus
branch
5 times, most recently
from
75d7793
to
ae9b4c1
Compare
aknuds1
changed the title
aknuds1
force-pushed
the
arve/revendor-prometheus
branch
2 times, most recently
from
b2212fc
to
9b33724
Compare
aknuds1
force-pushed
the
arve/revendor-prometheus
branch
from
f13aa08
to
e95b66e
Compare
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
aknuds1
force-pushed
the
arve/revendor-prometheus
branch
from
2bdb56f
to
ea2efbe
Compare
There was a problem hiding this comment.
I managed to look over the changes in the ingester, tenantfederation, and frontend/* packages. Unfortunately my battery will die soon, so I will need to get back to the rest later.
There were quite a few stylistic changes that make this already large change set even larger. For me at least this makes the review more error-prone. Perhaps doing them in a follow-up PR would have been easier to review. I don't think you should undo them at this point, but wanted to point this out
| @@ -27,7 +27,7 @@ import ( | |||
| // NewQueryable returns a queryable that iterates through all the tenant IDs | |||
| // that are part of the request and aggregates the results from each tenant's | |||
| // Querier by sending of subsequent requests. | |||
| // By setting bypassWithSingleQuerier to true the mergeQuerier gets bypassed | |||
| // By setting bypassWithSingleID to true the mergeQuerier gets bypassed | |||
There was a problem hiding this comment.
i also don't understand the need for this set of changes, but i suppose they're not wrong. Can you also update the next line? It still talks about a querier.
There was a problem hiding this comment.
@dimitarvdimitrov changing to bypassWithSingleID makes sense, since the number of queriers had to be abstracted away (only GEM will need multiple queriers). The doc strings need fixing up. I was waiting for Peter's input first :)
| ) | ||
| func NewQueryable(upstream storage.Queryable, bypassWithSingleID bool, maxConcurrency int, logger log.Logger) storage.Queryable { | ||
| callbacks := MergeQuerierCallbacks{ | ||
| IDs: func(ctx context.Context) ([]string, error) { |
There was a problem hiding this comment.
wy do we need to call this via a callback? Can't we just call tenants.TenantIDs(ctx) in each of LabelValues, LabelNames, and Select?
There was a problem hiding this comment.
It's necessary to abstract since the enterprise code needs to do it differently (hence the callbacks). I'll give it another look to make sure I'm not missing something.
|
|
||
| // tenantQuerier implements MergeQuerierUpstream, wrapping a storage.Querier. | ||
| // The federation ID gets injected into the context as a tenant ID. | ||
| type tenantQuerier struct { |
There was a problem hiding this comment.
can't we just call user.InjectOrgID and inject the tenatn id in the context in the three call sites of tenantQuerier's methods? like here
If that's possible, then we can remove tenantQuerier, MergeQuerierUpstream and MergeQuerierCallbacks.
There was a problem hiding this comment.
We can't just call user.InjectOrgID, because the enterprise product doesn't deal with federation IDs in the same way. I.e., it doesn't deal with tenant IDs in the context.
|
looks like others are happy with the change; i couldn't find any bugs in the code i've reviewed so far, so if you're confident, you can merge this without waiting for my full review (which might only come next week or on Sunday) |
@dimitarvdimitrov are you referring to the |
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
| @@ -2214,7 +2212,9 @@ func (i *Ingester) createTSDB(_ context.Context, userID string, walReplayConcurr | |||
| // this will actually create the blocks. If there is no data (empty TSDB), this is a no-op, although | |||
| // local blocks compaction may still take place if configured. | |||
| level.Info(userLogger).Log("msg", "Running compaction after WAL replay") | |||
| err = db.Compact() | |||
| // Note that we want to let TSDB creation finish without being interrupted by eventual context cancellation, | |||
There was a problem hiding this comment.
I don't think this is necessarily true. createTSDB is also done when ingester is starting, and if we get signal to stop ingester in that time, we don't need to wait until compaction finishes.
But I think it may be safer to wait until Compact() has done. I don't think there's a urgency of shutting down ingester, but there's an high importancy of doing it safely.
I personally agree with the comment and using context.Background() to not introduce more chaos to Compact().
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
In Ingester.QueryStream, ignore context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, log it with full diagnostics. Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
…or chunk queriers (#6408) * Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers In Ingester.QueryStream, support ignoring of context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, (span) log it with full diagnostics. Also wrap some chunk querying errors. --------- Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
…or chunk queriers (#6408) * Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers In Ingester.QueryStream, support ignoring of context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, (span) log it with full diagnostics. Also wrap some chunk querying errors. --------- Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> (cherry picked from commit f737a96)
…or chunk queriers (#6408) (#6440) * Ingester.QueryStream: Add support for ignoring context cancellation for chunk queriers In Ingester.QueryStream, support ignoring of context cancellation wrt. chunk queriers, the way it worked prior to #6085. If the context is canceled though, (span) log it with full diagnostics. Also wrap some chunk querying errors. --------- Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com> (cherry picked from commit f737a96) Co-authored-by: Arve Knudsen <arve.knudsen@gmail.com>
What this PR does
Update mimir-prometheus to latest main, i.e. 320f0c9c4a88.
The main change in Prometheus is that the context argument is moved from
storage.Queryable.Querierand instead to individual query methods (Select,LabelValues,LabelNames). This requires some adapting of Mimir code, especially that which relies on the context being a querier field (e.g. tenant federation).Also, due to (certain) Prometheus query methods gaining a context argument, they now respect context cancellation properly, unlike before.
TODO:
Which issue(s) this PR fixes or relates to
Fixes #5314.
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]