Configurable active user series

[pstibrany]

What this PR does: This PR makes tracking of active series in the blocks storage configurable (there are two three parameters: how often to purge old series from memory and update metrics, and how old must series be to be considered inactive, and one to enable active series tracking [disabled by default]).

This makes it possible to track active series more precisely even if they are still in the TSDB head, but haven't been appended any samples recently.

In the current state of the PR, this is only wired into the blocks engine, and not chunks engine.

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pracucci]

Thanks for working on it! I have few concerns:

1. The new metric adds more cognitive load to users when trying to understand the differences between chunks and blocks storage. Don't have an answer, but if it could work for the chunks storage too would probably easier to understand.
2. I'm a bit scared of the lock contention in ActiveSeries (because it's an exclusive lock). I'm wondering if we switch the entry timestamp to an atomic, if a RWMutex could be used (and most of the time we would acquire the read because we expect the series to be already in the map).

[pracucci]

I would default to 1m, to have more accurate tracking (by default).

[pstibrany]

Thanks for working on it! I have few concerns:

1. The new metric adds more cognitive load to users when trying to understand the differences between chunks and blocks storage. Don't have an answer, but if it could work for the chunks storage too would probably easier to understand.

I agree. Let's do that. (Reason why I kept chunks out of this PR is basically to first see how well it works in practice.)

2. I'm a bit scared of the lock contention in ActiveSeries (because it's an exclusive lock). I'm wondering if we switch the entry timestamp to an atomic, if a RWMutex could be used (and most of the time we would acquire the read because we expect the series to be already in the map).

I will write a benchmark to see throughput of both approaches.

[pstibrany]

Benchmark showing improvements when using RW lock and atomics to update timestamps.

name                                  old time/op    new time/op    delta
ActiveSeriesTest_single_series/50-4      154ns ± 2%      76ns ± 1%  -50.52%  (p=0.008 n=5+5)
ActiveSeriesTest_single_series/100-4     156ns ± 1%      76ns ± 1%  -51.25%  (p=0.008 n=5+5)
ActiveSeriesTest_single_series/500-4     158ns ± 1%      76ns ± 2%  -52.01%  (p=0.008 n=5+5)

name                                  old alloc/op   new alloc/op   delta
ActiveSeriesTest_single_series/50-4      0.00B          0.00B          ~     (all equal)
ActiveSeriesTest_single_series/100-4     0.00B          0.00B          ~     (all equal)
ActiveSeriesTest_single_series/500-4     0.00B          0.00B          ~     (all equal)

name                                  old allocs/op  new allocs/op  delta
ActiveSeriesTest_single_series/50-4       0.00           0.00          ~     (all equal)
ActiveSeriesTest_single_series/100-4      0.00           0.00          ~     (all equal)
ActiveSeriesTest_single_series/500-4      0.00           0.00          ~     (all equal)

[pracucci]

I think there's a race condition: we get a reference of an activeSeriesEntry and then we do update it. In the meanwhile, the purge could remove it, because the read lock is kept only during findEntryForSeries(). However, the next remote write push will add it back, right? If so, LGTM, but I would like your opinion too.

[pstibrany]

Well spotted. Should be fixed now. (There was another problem... since atomic integer was not a pointer, but part of "entry", which was stored in a slice as a plain struct, without pointer, it could actually move in memory when slice grows. That's why atomic integers are now pointers).

[pracucci]

There was another problem... since atomic integer was not a pointer, but part of "entry", which was stored in a slice as a plain struct, without pointer, it could actually move in memory when slice grows. That's why atomic integers are now pointers

Well spotted!

[gouthamve]

The new metric adds more cognitive load to users when trying to understand the differences between chunks and blocks storage. Don't have an answer, but if it could work for the chunks storage too would probably easier to understand.

I'd be okay adding this to chunks store too. How much overhead will this add?

[pstibrany]

The new metric adds more cognitive load to users when trying to understand the differences between chunks and blocks storage. Don't have an answer, but if it could work for the chunks storage too would probably easier to understand.

I'd be okay adding this to chunks store too. How much overhead will this add?

By my counting, each activeSeriesEntry needs 40 bytes on its own + copy of the labels. For 1M series that's at least 40 MB, and if we estimate labels to take 200 bytes per series, that would be additional 200 MB.

(In blocks storage, we try to reuse labels copy if added to the ref cache at the same time. In chunks doing the same is not easy.)

[pracucci]

Excellent job. I left a couple of nits. As discussed offline, I would add a enabled flag (default to false) to not let every Cortex user paying the cost of this tracking if not required (this is usually required by vendors billing systems).

[pracucci]

There was another problem... since atomic integer was not a pointer, but part of "entry", which was stored in a slice as a plain struct, without pointer, it could actually move in memory when slice grows. That's why atomic integers are now pointers

Well spotted!

[pstibrany]

Excellent job. I left a couple of nits. As discussed offline, I would add a enabled flag (default to false) to not let every Cortex user paying the cost of this tracking if not required (this is usually required by vendors billing systems).

Added -ingester.active-series-enabled flag (defaults to false), and switched fingerprinting to use xxHash 64-bit, which is faster than fnv64a (more visible for bigger labels, so benchmark now uses bigger label), but requires extra small allocation:

name                         old time/op    new time/op    delta
ActiveSeries_UpdateSeries-4    1.03µs ± 6%    0.74µs ±17%  -28.25%  (p=0.008 n=5+5)

name                         old alloc/op   new alloc/op   delta
ActiveSeries_UpdateSeries-4      206B ± 0%      253B ±11%  +23.01%  (p=0.008 n=5+5)

name                         old allocs/op  new allocs/op  delta
ActiveSeries_UpdateSeries-4      2.00 ± 0%      3.00 ± 0%  +50.00%  (p=0.008 n=5+5)

[pstibrany]

I've also made sure that if -ingester.active-series-enabled is false, then new metric doesn't appear in the metrics at all. Previous version of the chunks-code actually exported 0 value for each user, which is useless. Unfortunately this made PR bigger :( (alternative would be to not register the metric into to metrics registry, but still create all in-memory structures).

[pracucci]

Good job, still LGTM! Left final nits.

[bboreham]

In the current state of the PR, this is only wired into the blocks engine, and not chunks engine.

Is this still true?

[pstibrany]

In the current state of the PR, this is only wired into the blocks engine, and not chunks engine.

Is this still true?

Not anymore. Also there is now a flag to enable this, which is disabled by default.