Mark series ephemeral in distributor based on series matchers in runtime config #3897

replay · 2023-01-09T18:05:00Z

What this PR does

This PR makes it possible to mark certain series as ephemeral, based on a list of label matchers that can be configured via the runtime config.
All the code in this PR is behind a feature flag marked as experimental.

The Distributor property EphemeralSeriesProvider should be easily replaceable to inject other ephemeral series provider implementations.

Which issue(s) this PR fixes or relates to

Related: #3884

replay · 2023-01-12T13:58:32Z

pkg/distributor/distributor.go

+		// No ephemeral series from ruler.
+		if req.Source == mimirpb.RULE {
+			return next(ctx, pushReq)
+		}


I'm not sure if we want this, seems like it could be considered a bug if it isn't well documented. WDYT @pstibrany

We will need some way to direct output from rules to persistent storage only, ignoring the config in distributors. This was easiest way, but we can try to come up with a different that fits the design better.

replay · 2023-01-12T14:16:20Z

pkg/distributor/distributor.go

@@ -197,6 +207,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet, logger log.Logger) {
 	f.Float64Var(&cfg.InstanceLimits.MaxIngestionRate, maxIngestionRateFlag, 0, "Max ingestion rate (samples/sec) that this distributor will accept. This limit is per-distributor, not per-tenant. Additional push requests will be rejected. Current ingestion rate is computed as exponentially weighted moving average, updated every second. 0 = unlimited.")
 	f.IntVar(&cfg.InstanceLimits.MaxInflightPushRequests, maxInflightPushRequestsFlag, 2000, "Max inflight push requests that this distributor can handle. This limit is per-distributor, not per-tenant. Additional requests will be rejected. 0 = unlimited.")
 	f.IntVar(&cfg.InstanceLimits.MaxInflightPushRequestsBytes, maxInflightPushRequestsBytesFlag, 0, "The sum of the request sizes in bytes of inflight push requests that this distributor can handle. This limit is per-distributor, not per-tenant. Additional requests will be rejected. 0 = unlimited.")
+	f.BoolVar(&cfg.EphemeralMetricsEnabled, "distributor.ephemeral-metrics-enabled", false, "Enable marking series as ephemeral based on the given matchers in the runtime config.")


Eventually we can remove this flag and just leave the feature always enabled, but while it's still experimental I prefer having a kill switch so that we can disable it easily for example if there's a bug in the distributor middleware.

pstibrany · 2023-01-13T07:42:00Z

pkg/distributor/distributor.go

+			for ix := 0; ix < len(req.Timeseries); ix++ {
+				ts := req.Timeseries[ix]
+
+				if !ephemeralChecker.IsEphemeral(ts.Labels) {


If we pass req.Source to IsEphemeral, then we can have checker decide whether series originated from ruler can be marked as ephemeral or not. That's probably better than having if req.Source == mimirpb.RULE check above. (Can be in separate PR)

Alternatively, we can pass the source to EphemeralChecker call. That would allow us to have a two separate sets of configs, but if someone wants the same config for API and Ruler, then they would need to duplicate them.

Interesting idea, I like it. So then the list of label matchers would have to be keyed by the source, kind of like:

overrides: anonymous: ephemeral_series_matchers: api: - __name__="metric_from_api" ruler: - __name__="metric_from_ruler" any: - __name__="always_ephemeral"

I implemented this in b8e8c76

pstibrany

Very nice job! I've left some comments that I'd like to see addressed (missing test, some clarifications) before merging.

pkg/distributor/distributor.go

pstibrany · 2023-01-13T09:36:11Z

pkg/distributor/distributor.go

+					continue
+				}
+
+				if cap(deleteTs) == 0 {


[nit] Normally we use len for such check, why is cap better here?

the reason why i chose cap() is because if cap() == 0 then the next operation is deleteTs = make([]int, 0, len(req.Timeseries)-ix), this will set the cap() to > 0 but not the len(), so the check and the operation which is decided based on the check are closer together. Relying on len() would work too, but the increased len() on a subsequent iteration is a less direct consequence because it only gets raised further down in the loop via append().

anyway, I'm happy to change it to len() if you prefer that.

This is just relying on the first boolean now instead of len or cap: https://github.com/grafana/mimir/pull/3897/files#diff-8e51742acf8f88fa2ae01855fb19125feb5c54c41023f1eaa8beded9f1b4bcc3R1063-R1076

This made sense because there are two separate actions that need to be taken on the first iteration, only one of which is directly related to the length of deleteTs.

pkg/distributor/distributor.go

pkg/util/ephemeral/interface.go

pkg/util/ephemeral/label_matchers.go

pkg/util/validation/limits.go

pkg/distributor/distributor.go

pstibrany · 2023-01-13T10:05:21Z

pkg/distributor/distributor.go

+			for ix := 0; ix < len(req.Timeseries); ix++ {
+				ts := req.Timeseries[ix]
+
+				if !ephemeralChecker.IsEphemeral(ts.Labels) {


Alternatively, we can pass the source to EphemeralChecker call. That would allow us to have a two separate sets of configs, but if someone wants the same config for API and Ruler, then they would need to duplicate them.

pracucci · 2023-01-16T09:40:29Z

docs/sources/mimir/reference-configuration-parameters/index.md

+
+# (experimental) Enable marking series as ephemeral based on the given matchers
+# in the runtime config.
+# CLI flag: -distributor.ephemeral-metrics-enabled


Here it's called "ephemeral metrics" while below it's called "ephemeral series". Pick one of the two, so we standardise naming.

thx, good catch. I started out calling it "ephemeral metrics", but then we changed the checker so it checks the entire labelset instead of only checking the metric name, so I'll rename it to "ephemeral series" everywhere.

done here: 52465b710c2c347b675cf7c2fb959a8c9631f9ac

pracucci

Good job! The overall design LGTM. I left Peter review the tiny bits in the logic ;)

pkg/util/validation/limits.go

pracucci · 2023-01-16T09:48:58Z

pkg/util/validation/limits.go

@@ -168,6 +169,8 @@ type Limits struct {
 	ForwardingEndpoint      string          `yaml:"forwarding_endpoint" json:"forwarding_endpoint" doc:"nocli|description=Remote-write endpoint where metrics specified in forwarding_rules are forwarded to. If set, takes precedence over endpoints specified in forwarding rules."`
 	ForwardingDropOlderThan model.Duration  `yaml:"forwarding_drop_older_than" json:"forwarding_drop_older_than" doc:"nocli|description=If set, forwarding drops samples that are older than this duration. If unset or 0, no samples get dropped."`
 	ForwardingRules         ForwardingRules `yaml:"forwarding_rules" json:"forwarding_rules" doc:"nocli|description=Rules based on which the Distributor decides whether a metric should be forwarded to an alternative remote_write API endpoint."`
+
+	EphemeralSeriesMatchers ephemeral.LabelMatchers `yaml:"ephemeral_series_matchers" json:"ephemeral_series_matchers" doc:"nocli|description=Matchers to check if a series should be marked ephemeral, matching series get marked to be stored in the ephemeral store." category:"experimental"`


Why the "description" here? If we set the description here, the CLI flag description won't be used, but CLI flag description is more detailed.

Why the "description" here?

I just followed the example of the line above.

I removed the description here: 3635f97f3

I'm trying to figure out how to make it use the description from the CLI flag.
Do you know if there's something else that I need to do apart from just removing this doc tag?

replay · 2023-01-16T11:46:36Z

Going to force-push to rebase on main.

pstibrany

Thank you, looks good to me!

pkg/distributor/distributor.go

pstibrany · 2023-01-16T14:53:29Z

pkg/mimir/modules.go

@@ -297,7 +297,7 @@ func (t *Mimir) initDistributorService() (serv services.Service, err error) {
 	// ruler's dependency)
 	canJoinDistributorsRing := t.Cfg.isAnyModuleEnabled(Distributor, Write, All)

-	t.Distributor, err = distributor.New(t.Cfg.Distributor, t.Cfg.IngesterClient, t.Overrides, t.ActiveGroupsCleanup, t.Ring, canJoinDistributorsRing, t.Registerer, util_log.Logger)
+	t.Distributor, err = distributor.New(t.Cfg.Distributor, t.Cfg.IngesterClient, t.Overrides, t.ActiveGroupsCleanup, t.Ring, t.Overrides, canJoinDistributorsRing, t.Registerer, util_log.Logger)


We want to be able to overrides ephemeral checker in GEM. To do that, we could do the following:

introduce t.EphemeralChecker field, and let initDistributorService use that field when constructing distributor

add new ephemeral-checker module, with initEphemeralChecker function setting t.EphemeralChecker = t.Overrides. Distributor service module needs to depend on this module.

in GEM we can then have yet another new module (say GemEphemeralChecker for this discussion) that depends on ephemeral-checker and sets t.EphemeralChecker to a new value. (We would need to set distributor-service to depend on our module, to make sure that init-methods are called in correct order: initEphemeralChecker, initGemEphemeralChecker and finally initDistributorService)

We can do this preparation in a separate PR if you want.

Nice idea, I haven't thought further than "inject it into the Distributor's constructor".
I think to keep the PRs small and simple I'd prefer to split that out into another PR, but I'll basically just follow your plan as you described.

pkg/util/ephemeral/label_matchers.go

pkg/util/ephemeral/interface.go

pstibrany · 2023-01-16T15:04:23Z

pkg/util/ephemeral/label_matchers.go

+type matcherSet []*labels.Matcher
+
+// matches checks whether all the matchers match the given label set.
+func (ms matcherSet) matches(lset []mimirpb.LabelAdapter) bool {


I don't see this bit implemented, so I assume that we will optimize it in separate PR.

pkg/util/validation/limits.go

pstibrany

Looks great, thank you very much for addressing my feedback.

pstibrany · 2023-01-17T08:28:09Z

pkg/util/ephemeral/interface.go

+	// EphemeralChecker returns an object which checks a series and decides whether
+	// this series should be ephemeral based on the given user's configuration.
+	// Can return nil if the user has no relevant configuration.
+	EphemeralChecker(string, mimirpb.WriteRequest_SourceEnum) SeriesChecker


[nit] I'd include names of parameters in this method, to better document their meaning. (It's not clear that first parameter is tenant)

ok, done here 9e338b7d7

pstibrany · 2023-01-17T08:32:12Z

pkg/util/ephemeral/label_matchers.go

+}
+
+// HasMatchers returns true if there is at least one matcher defined, otherwise it returns false.
+func (m *MatcherSetsForSource) HasMatchers() bool {


[nit] There doesn't seem to be a good reason why m needs to be a pointer. (No method of MatcherSetsForSource modifies m)

thx, done: c56aaa0cd

pracucci · 2023-01-17T09:28:10Z

docs/sources/mimir/reference-configuration-parameters/index.md

+# (experimental) Enable marking series as ephemeral based on the given matchers
+# in the runtime config.
+# CLI flag: -distributor.ephemeral-series-enabled
+[ephemeral_metrics_enabled: <boolean> | default = false]


The YAML config option is still "metrics". Please change it to "series" too.

sorry for missing those... fixed: 705f50fcb663fa3cd31021a0187f3dda532501c1

pracucci · 2023-01-17T09:28:45Z

docs/sources/mimir/reference-configuration-parameters/index.md

@@ -2826,6 +2831,8 @@ The `limits` block configures default and per-tenant limits imposed by component
 # Rules based on which the Distributor decides whether a metric should be
 # forwarded to an alternative remote_write API endpoint.
 [forwarding_rules: <map of string to validation.ForwardingRule> | default = ]
+
+ephemeral_series_matchers:


The documentation is empty here. Can you fix it, please?

Looking into why this doesn't pick up the description from the CLI flag definition...

Fixed here, I needed to update the handling of custom types in the doc-generator to make it work: 01b5685

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the mark_series_ephemeral_in_distributor branch 2 times, most recently from dfaeb66 to e323359 Compare January 10, 2023 12:01

replay changed the title ~~WIP Mark series ephemeral in distributor based on data in kvstore~~ Mark series ephemeral in distributor based on data in kvstore Jan 10, 2023

replay changed the title ~~Mark series ephemeral in distributor based on data in kvstore~~ Mark series ephemeral in distributor based on ephemeral metrics config in kvstore Jan 10, 2023

replay marked this pull request as ready for review January 10, 2023 14:12

replay requested review from a team as code owners January 10, 2023 14:12

replay requested a review from pstibrany January 10, 2023 14:13

replay marked this pull request as draft January 10, 2023 14:29

pstibrany mentioned this pull request Jan 10, 2023

Ephemeral storage #3884

Closed

replay force-pushed the mark_series_ephemeral_in_distributor branch 5 times, most recently from 01583c8 to 908cc58 Compare January 12, 2023 13:20

replay changed the title ~~Mark series ephemeral in distributor based on ephemeral metrics config in kvstore~~ Mark series ephemeral in distributor based on ephemeral series matchers in runtime config Jan 12, 2023

replay commented Jan 12, 2023

View reviewed changes

replay marked this pull request as ready for review January 12, 2023 14:26

replay changed the title ~~Mark series ephemeral in distributor based on ephemeral series matchers in runtime config~~ Mark series ephemeral in distributor based on series matchers in runtime config Jan 12, 2023

pstibrany reviewed Jan 13, 2023

View reviewed changes

pracucci reviewed Jan 16, 2023

View reviewed changes

replay force-pushed the mark_series_ephemeral_in_distributor branch from 2a6013f to 169acf2 Compare January 16, 2023 11:46

pstibrany approved these changes Jan 16, 2023

View reviewed changes

pstibrany approved these changes Jan 17, 2023

View reviewed changes

pracucci reviewed Jan 17, 2023

View reviewed changes

replay added 26 commits January 17, 2023 16:28

naming and adding tests

26e55fc

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

linter and test fixes

399f99e

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

configure ephemeral matchers per source

fbb3182

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

pass ephemeral checker into distributors New()

27f0f0a

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

update reference help

40509be

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

check len before calling util.RemoveSliceIndexes()

b89d5e2

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

address PR feedback

01b15b1

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

only get timeseries slice for ephemeral timeseries when necessary

d1e4eb2

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

rename ephemeral metrics to series

1295a15

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

add unit tests for ShouldMarkEphemeral()

36878da

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

generate list of possible source strings, and linter

d3db6d6

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

shortcut ephemeral middleware if no matchers are defined

60e06df

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

remove "description" tag, fix test

a477cad

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

remove unnecessary HasMatchers() method

f1f71bb

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

dont export ephemeralChecker

b9a3612

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

no need to check nil

229a7e4

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

create different ephemeral checkers per source

da42b85

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

fix marshaling of labelsets and remove unnecessary type

ca4c951

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

remove unnecessary map of source strings

f1f84e3

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

changelog entry

e1da1b6

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

name interface parameters

2d05418

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

MatcherSetsForSource doesnt need pointer receiver

7d24e23

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

fix naming of yaml parameter metrics->series

138166b

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

LabelMatchers.Set() should trim parameter

8510a31

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

add comment

cc1d8e8

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

fix generation of reference configuration parameters

51781c8

Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>

replay force-pushed the mark_series_ephemeral_in_distributor branch from 01b5685 to 51781c8 Compare January 17, 2023 15:29

replay merged commit 3580f8f into main Jan 18, 2023

replay deleted the mark_series_ephemeral_in_distributor branch January 18, 2023 14:05

replay mentioned this pull request Jan 18, 2023

introduce ephemeral checker service and make distributor service depend on it #3997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark series ephemeral in distributor based on series matchers in runtime config #3897

Mark series ephemeral in distributor based on series matchers in runtime config #3897

replay commented Jan 9, 2023 •

edited

Loading

replay Jan 12, 2023

pstibrany Jan 12, 2023

replay Jan 12, 2023

pstibrany Jan 13, 2023

pstibrany Jan 13, 2023

replay Jan 13, 2023 •

edited

Loading

replay Jan 13, 2023

pstibrany left a comment

pstibrany Jan 13, 2023

replay Jan 13, 2023 •

edited

Loading

replay Jan 16, 2023

pstibrany Jan 13, 2023

pracucci Jan 16, 2023

replay Jan 16, 2023

replay Jan 16, 2023

pracucci left a comment

pracucci Jan 16, 2023

replay Jan 16, 2023

replay Jan 17, 2023

replay commented Jan 16, 2023

pstibrany left a comment

pstibrany Jan 16, 2023

replay Jan 16, 2023

pstibrany Jan 16, 2023

pstibrany left a comment

pstibrany Jan 17, 2023

replay Jan 17, 2023

pstibrany Jan 17, 2023 •

edited

Loading

replay Jan 17, 2023

pracucci Jan 17, 2023

replay Jan 17, 2023

pracucci Jan 17, 2023

replay Jan 17, 2023

replay Jan 17, 2023

Mark series ephemeral in distributor based on series matchers in runtime config #3897

Mark series ephemeral in distributor based on series matchers in runtime config #3897

Conversation

replay commented Jan 9, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay commented Jan 16, 2023

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

replay commented Jan 9, 2023 •

edited

Loading

replay Jan 13, 2023 •

edited

Loading

replay Jan 13, 2023 •

edited

Loading

pstibrany Jan 17, 2023 •

edited

Loading