Add telemetry for TS command
#135471
Add telemetry for TS command
#135471
Conversation
kkrik-es
added
>non-issue
Team:StorageEngine
:StorageEngine/ES|QL
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
|
|
There was a problem hiding this comment.
Thanks @kkrik-es
I left two comments, my main concern is that we are tracking FROM commands that don't really exist in the query. For some cases the fix could be easy, but for others it could require some refactoring.
Happy to chat about it off-line, if it can help
...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/telemetry/VerifierMetricsTests.java
Outdated
Show resolved
Hide resolved
| public void testTimeSeriesAggregate() { | ||
| assumeTrue("TS required", EsqlCapabilities.Cap.TS_COMMAND_V0.isEnabled()); | ||
| Counters c = esql(""" | ||
| TS metrics |
There was a problem hiding this comment.
Does it also work for a query without stats?
Eg. will this return ts:1, keep:1, from:0?
TS metrics | KEEP @timestamp
I didn't test it, but if I remember well it gets planned as a normal FROM, so I expect it to return from:1, ts:0
Could you please add a test and verify?
If it's how I suspect, a fix could be problematic, as after analysis we lose the information we stored in the UnresolvedRelation to distinguish the two situations.
cc @astefan this is another case that would require the refactoring we discussed some time ago, ie. move the stats calculation to parse time, and unify it with APM telemetry
There was a problem hiding this comment.
Actually, an easy fix could be to rely on indexMode in EsRelation, ie.
FROM(x -> x instanceof EsRelation r && r.indexMode() != IndexMode.TIME_SERIES),
TS(x -> x instanceof EsRelation r && r.indexMode() == IndexMode.TIME_SERIES),
There was a problem hiding this comment.
No that's not accurate, FROM is also applicable to time-series indices. In general, TS makes sense when combined with STATS, otherwise it doesn't offer much and it's actually more restrictive than it should. In that respect, I think it's ok if we don't track TS metrics | KEEP @timestamp as TS but rather as FROM.
There was a problem hiding this comment.
Looking at the code, I see UnresolvedRelation is built with IndexMode.STANDARD from FROM and with IndexMode.TIME_SERIES for TS
Then we build the EsRelation starting from the UnresolvedRelation, using the same index mode.
At optimization time things can change, but here we are still at verification phase, so we should have the original information.
Am I missing something?
There was a problem hiding this comment.
Hm actually you're right, this seems to work! @dnhatn to double-check it too.
elasticsearchmachine
added
the
serverless-linked
* upstream/main: (22 commits) Fix InternalCategorizationAggregationTests.testReduceRandom (elastic#135533) [DOCS] GeoIP processor: add clarification about using a reverse proxy endpoint (elastic#135534) Move `ProjectRoutingInfo` and related classes (elastic#135586) Refactor IndexAbstractionResolver (elastic#135587) Simplify returnLocalAll handling in ES|QL (elastic#135353) Reapply "Add an option to return early from an allocate call" (elastic#135589) Mute org.elasticsearch.xpack.esql.qa.single_node.GenerativeIT test elastic#134407 Mute org.elasticsearch.xpack.esql.heap_attack.HeapAttackIT testAggTooManyMvLongs elastic#135585 Mute org.elasticsearch.multiproject.test.XpackWithMultipleProjectsClientYamlTestSuiteIT test {yaml=esql/60_usage/Basic ESQL usage output (telemetry) snapshot version} elastic#135579 Mute org.elasticsearch.search.ccs.KnnVectorQueryBuilderCrossClusterSearchIT testKnnQueryWithCcsMinimizeRoundTripsFalse elastic#135573 Mute org.elasticsearch.xpack.esql.inference.textembedding.TextEmbeddingOperatorTests testSimpleCircuitBreaking elastic#135569 Add telemetry for `TS` command (elastic#135471) Mute org.elasticsearch.cluster.routing.allocation.decider.RestoreInProgressAllocationDeciderTests testCanAllocatePrimaryExistingInRestoreInProgress elastic#135566 allocation: clarify RestoreInProgressAllocationDecider failure message (elastic#132307) [ES|QL] Register AggregateMetricDoubleLiteral (elastic#135054) Validate Logstash pipeline ID when creating. (elastic#135378) Migrate transport versions 8841050 through 8841041 (elastic#135555) Mute org.elasticsearch.search.ccs.SparseVectorQueryBuilderCrossClusterSearchIT testSparseVectorQueryWithCcsMinimizeRoundTripsFalse elastic#135559 Mute org.elasticsearch.action.admin.cluster.stats.SearchUsageStatsTests testToXContent elastic#135558 Testing indices query cache memory stats (elastic#135298) ...
4 participants
Fixes #135441
Fixes #133449