Skip to content
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

Set receiver chunk trimming as a feature flag #7884

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
- [#7855](https://github.com/thanos-io/thanos/pull/7855) Compcat/Query: Add support for comma separated replica labels.
- [#7654](https://github.com/thanos-io/thanos/pull/7654) *: Add '--grpc-server-tls-min-version' flag to allow user to specify TLS version, otherwise default to TLS 1.3
- [#7854](https://github.com/thanos-io/thanos/pull/7854) Query Frontend: Add `--query-frontend.force-query-stats` flag to force collection of query statistics from upstream queriers.
- [#7884](https://github.com/thanos-io/thanos/pull/7884) Receive: Add a `disable-chunk-trimming` feature which can be used to return entire chunks to queriers instead of cutting them to the requested range.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would users know whether to enable this or not? Does this actually change the results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to run cortex integration tests locally using the official docs so I cannot reproduce the issue and verify why the test fails. @yeya24 do you happen to have the test case which reproduces this failure?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7815 (comment)
I only have the query here but I see it can fail for either start() or end() with timestamp function.

Maybe we can mention the impacted query string in the changelog? If they don't run similar query they can enable the flag

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But once the chunks are in object storage, the result changes because Store doesn't trim them? So, it sounds like we always need to disable trimming.


### Changed

Expand Down
25 changes: 19 additions & 6 deletions cmd/thanos/receive.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,15 @@ import (
"github.com/thanos-io/thanos/pkg/tls"
)

type feature string

const (
metricNamesFilter feature = "metric-names-filter"
disableReceiverChunkTrimming feature = "disable-chunk-trimming"
)

const (
compressionNone = "none"
metricNamesFilter = "metric-names-filter"
compressionNone = "none"
)

func registerReceive(app *extkingpin.App) {
Expand Down Expand Up @@ -141,11 +147,17 @@ func runReceive(

level.Info(logger).Log("mode", receiveMode, "msg", "running receive")

multiTSDBOptions := []receive.MultiTSDBOption{}
for _, feature := range *conf.featureList {
if feature == metricNamesFilter {
var multiTSDBOptions []receive.MultiTSDBOption
for _, f := range *conf.featureList {
switch feature(f) {
case metricNamesFilter:
multiTSDBOptions = append(multiTSDBOptions, receive.WithMetricNameFilterEnabled())
level.Info(logger).Log("msg", "metric name filter feature enabled")
case disableReceiverChunkTrimming:
multiTSDBOptions = append(multiTSDBOptions, receive.DisableReceiverChunkTrimming())
level.Info(logger).Log("msg", "disable receiver chunk trimming feature enabled")
default:
return errors.Errorf("unknown feature %q", f)
}
}

Expand Down Expand Up @@ -1036,7 +1048,8 @@ func (rc *receiveConfig) registerFlag(cmd extkingpin.FlagClause) {
cmd.Flag("receive.limits-config-reload-timer", "Minimum amount of time to pass for the limit configuration to be reloaded. Helps to avoid excessive reloads.").
Default("1s").Hidden().DurationVar(&rc.limitsConfigReloadTimer)

rc.featureList = cmd.Flag("enable-feature", "Comma separated experimental feature names to enable. The current list of features is "+metricNamesFilter+".").Default("").Strings()
features := []string{string(metricNamesFilter), string(disableReceiverChunkTrimming)}
rc.featureList = cmd.Flag("enable-feature", "Comma separated experimental feature names to enable. The current list of features is "+strings.Join(features, ",")+".").Strings()
}

// determineMode returns the ReceiverMode that this receiver is configured to run in.
Expand Down
5 changes: 3 additions & 2 deletions docs/components/receive.md
Original file line number Diff line number Diff line change
Expand Up @@ -359,9 +359,10 @@ Flags:
detected maximum container or system memory.
--enable-auto-gomemlimit Enable go runtime to automatically limit memory
consumption.
--enable-feature= ... Comma separated experimental feature names
--enable-feature=ENABLE-FEATURE ...
Comma separated experimental feature names
to enable. The current list of features is
metric-names-filter.
metric-names-filter,disable-chunk-trimming.
--grpc-address="0.0.0.0:10901"
Listen ip:port address for gRPC endpoints
(StoreAPI). Make sure this address is routable
Expand Down
14 changes: 12 additions & 2 deletions pkg/receive/multitsdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ type MultiTSDB struct {
exemplarClients map[string]*exemplars.TSDB
exemplarClientsNeedUpdate bool

metricNameFilterEnabled bool
metricNameFilterEnabled bool
disableReceiverChunkTrimming bool
}

// MultiTSDBOption is a functional option for MultiTSDB.
Expand All @@ -81,6 +82,13 @@ func WithMetricNameFilterEnabled() MultiTSDBOption {
}
}

// DisableReceiverChunkTrimming disables trimming when reading chunks from the TSDB.
func DisableReceiverChunkTrimming() MultiTSDBOption {
return func(s *MultiTSDB) {
s.disableReceiverChunkTrimming = true
}
}

// NewMultiTSDB creates new MultiTSDB.
// NOTE: Passed labels must be sorted lexicographically (alphabetically).
func NewMultiTSDB(
Expand Down Expand Up @@ -727,7 +735,9 @@ func (t *MultiTSDB) startTSDB(logger log.Logger, tenantID string, tenant *tenant
shipper.DefaultMetaFilename,
)
}
options := []store.TSDBStoreOption{}
var options = []store.TSDBStoreOption{
store.WithChunkTrimming(!t.disableReceiverChunkTrimming),
}
if t.metricNameFilterEnabled {
options = append(options, store.WithCuckooMetricNameStoreFilter())
}
Expand Down
9 changes: 8 additions & 1 deletion pkg/store/tsdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ func WithCuckooMetricNameStoreFilter() TSDBStoreOption {
}
}

func WithChunkTrimming(enableChunkTrimming bool) TSDBStoreOption {
return func(s *TSDBStore) {
s.trimChunks = enableChunkTrimming
}
}

// TSDBStore implements the store API against a local TSDB instance.
// It attaches the provided external labels to all results. It only responds with raw data
// and does not support downsampling.
Expand All @@ -62,6 +68,7 @@ type TSDBStore struct {
component component.StoreAPI
buffers sync.Pool
maxBytesPerFrame int
trimChunks bool

extLset labels.Labels
startStoreFilterUpdate bool
Expand Down Expand Up @@ -275,7 +282,7 @@ func (s *TSDBStore) Series(r *storepb.SeriesRequest, seriesSrv storepb.Store_Ser
Start: r.MinTime,
End: r.MaxTime,
Limit: int(r.Limit),
DisableTrimming: true,
DisableTrimming: !s.trimChunks,
}
set := q.Select(srv.Context(), true, hints, matchers...)

Expand Down
Loading