ESQL: telemetry with inlinestats #134309
ESQL: telemetry with inlinestats #134309
Conversation
astefan
added
>bug
test-release
|
Hi @astefan, I've created a changelog YAML for you. |
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| b.set(STATS.ordinal()); | ||
| } else { | ||
| plan.forEachDownMayReturnEarly((p, breakEarly) -> { | ||
| if (p instanceof UnaryPlan up && up.child() instanceof Aggregate && p instanceof InlineStats == false) { |
There was a problem hiding this comment.
Would this track correctly an Aggregate child of a binary plan? ... | STATS ... | LOOKUP JOIN ...
(Slightly off topic: this is one more case where a back reference to the parent would help. Don't know if we ever considered it.)
An alternative might be to just count Aggregate and InlineStats and subtract agg's count by inlinestats' and only set the bit if the result is non-zero. Don't know if it'd be prettier, though.
There was a problem hiding this comment.
Would this track correctly an Aggregate child of a binary plan?
I think it will, forEachDownMayReturnEarly applies to all the children
There was a problem hiding this comment.
Unfortunately, it doesn't correctly count the Aggregates in that specific case @luigidellaquila. @bpintea is correct, I rushed this PR. Thank you for spotting this.
There was a problem hiding this comment.
Right, my bad, now I see it. The case where the Aggregate is a direct child of an N-ary plan
There was a problem hiding this comment.
Thanks @astefan, LGTM
I wonder if, in the long term, we could refactor this and unify it with APM telemetry, accounting for commands at parse time.
It would save us from dealing with special cases like this one, where we build complex plans from a single command.
On the first look, it can help us, but it needs a deep dive to see if there are any things we are missing. Thank you for this suggestion, if you believe it's worth a github issue not to lose track of it, please go ahead and create one. Catching issues like that one boils down to two main things: having a test for it (which is really hard given the wide range of queries telemetry is used for) which unfortunately it's also the case of counting metrics at parsing time, having a good imagination for things to try out and know the code that could lead to an issue. |
…inlinestats_telemetry
…asticsearch into inlinestats_telemetry
| assertEquals(0, dissect(c)); | ||
| assertEquals(0, eval(c)); | ||
| assertEquals(0, grok(c)); | ||
| assertEquals(0, limit(c)); | ||
| assertEquals(0, sort(c)); | ||
| assertEquals(0, stats(c)); | ||
| assertEquals(1L, where(c)); | ||
| assertEquals(0, enrich(c)); | ||
| assertEquals(0, mvExpand(c)); | ||
| assertEquals(0, show(c)); | ||
| assertEquals(0, row(c)); | ||
| assertEquals(1L, from(c)); | ||
| assertEquals(0, drop(c)); | ||
| assertEquals(0, keep(c)); | ||
| assertEquals(0, rename(c)); | ||
| assertEquals(1L, inlinestats(c)); | ||
| assertEquals(0, lookupjoin(c)); | ||
| assertEquals(1, function("max", c)); |
There was a problem hiding this comment.
It might be worth at some point to refactor this list and somehow only provide the non-zero counters to a checking function that would also assert on all the rest being 0. Maybe when we'd consider tapping the parser for usage.
astefan
added
the
auto-merge-without-approval
4 participants
No description provided.