Conversation
…d stop all flushing from ingesters
mapno
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
joe-elliott,
knylander-grafana,
mdisibio,
stoewer,
yvrhdn and
zalegrala
as code owners
Introduced a new `RF1After` field in the search request to allow filtering blocks by replication factor and time. Updated backend logic and tests to handle this functionality. Adjusted max search duration to enhance usability and clarity in search tests. Signed-off-by: Mario <mariorvinas@gmail.com>
Signed-off-by: Mario <mariorvinas@gmail.com>
modules/querier/config.go
Outdated
|
|
||
| type TraceByIDConfig struct { | ||
| QueryTimeout time.Duration `yaml:"query_timeout"` | ||
| RF1After time.Time `yaml:"rf1_after"` |
There was a problem hiding this comment.
Rethinking the separate settings for search and trace by id rf1. I don't think there is a case to enable them separately - but I'm remembering now this was done because we don't have a great way to share a config option between frontend and querier modules (frontend has knowledge of blocks during search, and querier has knowledge of blocks for trace by id). Can you think of a better approach?
There was a problem hiding this comment.
Now rf1After is passed via query params to the querier.
Added RF1After field to trace query validation logic and related structures. Updated request parsing, test cases, and query configuration to support RF1After. Removed deprecated RF1After config from querier module, ensuring runtime determination. Signed-off-by: Mario <mariorvinas@gmail.com>
modules/frontend/tag_sharder.go
Outdated
| endT := time.Unix(int64(end), 0) | ||
| blocks := blockMetasForSearch(s.reader.BlockMetas(tenantID), startT, endT, backend.DefaultReplicationFactor) | ||
| blocks := blockMetasForSearch(s.reader.BlockMetas(tenantID), startT, endT, func(m *backend.BlockMeta) bool { | ||
| return m.ReplicationFactor == backend.DefaultReplicationFactor |
There was a problem hiding this comment.
Can we not use the same logic here as in the search sharder?
There was a problem hiding this comment.
Actually, we need to. Otherwise these requests will fail when we stop flushing ingester blocks. Great catch!
Signed-off-by: Mario <mariorvinas@gmail.com>
|
|
||
| // validate start and end parameter | ||
| _, _, _, _, _, reqErr := api.ValidateAndSanitizeRequest(req) | ||
| _, _, _, _, _, _, reqErr := api.ValidateAndSanitizeRequest(req) |
There was a problem hiding this comment.
This function signature is getting lengthy. Do we want to introduce a TraceByIDRequest object and use it as the output from api.ParseTraceID(req) ?
There was a problem hiding this comment.
Yes, I thought of that as well. What didn't convince me was that it also contains extra params start and end that are not part of TraceByIDRequest and it requires extra changes.
I don't have a strong opinion, but initially favored introducing only the necessary changes.
3 participants
What this PR does:
Updates #4478 to actual state of
main. Original text:These are the changes needed to migrate the backend read path to RF1. New config options
rf1_afterwhich is a timestamp, after that only blocks with rf==1 are included in searches and trace by ID lookups. And an option to discontinue flushes to object storage from the ingesters.How each api is handled:
Search: the frontend determines the blocks:
Trace lookup: the querier determines the blocks:
Tags: this is actually not filtering on replication factor, so no changes needed
Metrics: already limited to rf1 blocks
The migration path:
rf1_afterto move the read path over to the new RF1 blocks (recent searches/tags/tracelookups are still directed at ingesters)Note: Global config is non-trivial due to Tempo's modular setup, so this adds redundant config in multiple places. Not ideal.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]