[Downsampling++] Add time series telemetry in xpack usage #134214
Conversation
gmarouli
added
>enhancement
:StorageEngine/Downsampling
|
Hi @gmarouli, I've created a changelog YAML for you. |
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| /* | ||
| * We now add a number of simulated data streams to the cluster state. We mix different combinations of: | ||
| * - time series and standard data streams & backing indices | ||
| * - lifecycle with or without downsampling |
There was a problem hiding this comment.
| * - lifecycle with or without downsampling | |
| * - DLM with or without downsampling |
| var downsamplingConfiguredBy = randomFrom(DownsampledBy.values()); | ||
| boolean isDownsampled = downsamplingConfiguredBy != DownsampledBy.NONE && isTimeSeriesDataStream; | ||
| // An index/data stream can have both ILM & DLM configured; by default, ILM "wins" | ||
| boolean hasLifecycle = usually() || (isDownsampled && downsamplingConfiguredBy == DownsampledBy.DLM); |
There was a problem hiding this comment.
Nit: usually is !rarely so it's almost always.. Maybe use randomDouble() < 0.8 or so, to make it more concrete?
| tsIndexCount, | ||
| ilmStats.getDownsamplingStats(), | ||
| ilmStats.getIlmPolicyStats(), | ||
| dlmStats.getDownsamplingStats(), |
There was a problem hiding this comment.
You mean why I picked this variable name?
There was a problem hiding this comment.
I thought this call is for ilmStats, surprised to see dlmStats for this arg.
There was a problem hiding this comment.
Ah I see. There are two different constructors one that accepts stats for both ILM and DLM and one that only accepts DLM for the serverless use case, this was syntactic sugar to make more explicit that ILM stats would be null.
There was a problem hiding this comment.
I will add a comment for clarity
| builder.field("phases_in_use", phasesUsedInDownsampling); | ||
| builder.endObject(); | ||
| } | ||
| builder.startObject("dlm"); |
There was a problem hiding this comment.
Should we also check for null dlmDownsamplingStats here?
There was a problem hiding this comment.
It doesn't hurt, I will add it.
| } | ||
| } | ||
|
|
||
| public record DownsamplingFeatureStats(long dataStreamsCount, long indexCount, long minRounds, double averageRounds, long maxRounds) |
There was a problem hiding this comment.
Are you planning to add stats on dataset size and reduction, later?
There was a problem hiding this comment.
Stats on dataset size and reduction require a lot more infrastructure that we do not have currently. If we do move ahead with these plans then yes, I think we should try to expose them here too.
kkrik-es
left a comment
kkrik-es
left a comment
There was a problem hiding this comment.
Nice and clean. Consider asking Martijn to take a look too.
gmarouli
added
the
auto-merge-without-approval
4 participants
In this PR we introduce telemetry for time series trying to answer the following questions:
Fixes: #133953