ES|QL: Add (stack) telemetry for views#143745
Conversation
|
Hi @luigidellaquila, I've created a changelog YAML for you. |
…ack' into esql/views_telemetry_stack
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
...k/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/telemetry/VerifierMetricsTests.java
Outdated
Show resolved
Hide resolved
| return child(); | ||
| } | ||
|
|
||
| public String name() { |
There was a problem hiding this comment.
If the original code kept name private and, also, to have an easier check for this - why not create a boolean method in Subquery that checks the name existence? isView or something similar.
Wondering also why Subquery was not extended with a new class - View - that added the name. It feels very fragile to check name inside Subquery to discover if a plan is or is not a View. My 2c.
Maybe @craigtaverner can weigh in on this aspect (unrelated to this PR).
There was a problem hiding this comment.
Thanks @astefan, I'm adding an isView() method, it will make the code much more readable, and maybe it will simplify a future refactoring.
I agree that having a subclass specific for views could make things more clear
There was a problem hiding this comment.
It reminds me of this PR, it adds ViewUnionAll that extends UnionAll. I wonder if it affects view and subquery's telemetry?
There was a problem hiding this comment.
I'm updating the PR at #143564 to remove the name field from Subquery. Instead the name is being saved inside the ViewUnionAll class, as @fang-xing-esql mentioned. I do actually also have a NamedSubquery class extending Subquery, but this is only ephemeral, and should not exist in the final plan.
There was a problem hiding this comment.
That PR merged, and so Subquery no longer has names.
There was a problem hiding this comment.
We should remove these changes to Subquery.
| if (metrics == null || viewResolution.viewQueries().isEmpty()) { | ||
| return; | ||
| } | ||
| // If any views were used, increment the views metric |
There was a problem hiding this comment.
I don't think this comment helps the readability of the code. I think it is obvious what the next line does :-).
| public void testSubqueryTelemetry() { | ||
| Row innerPlan = new Row(Source.EMPTY, List.of()); | ||
|
|
||
| // A named Subquery represents a view - it should be excluded from plan traversal telemetry |
There was a problem hiding this comment.
Imho, this method also has too many comments.
craigtaverner
left a comment
There was a problem hiding this comment.
I think the changes to Subquery will make this easier. Let's get that in and simplify this one.
docs/changelog/143745.yaml
Outdated
| issues: [] | ||
| pr: 143745 | ||
| summary: Add (stack) telemetry for views | ||
| type: enhancement |
There was a problem hiding this comment.
I'm wondering, do we add changelog entries for unreleased (snapshot-ony) features? Perhaps the label should be >non-issue?
| COMPLETION(Completion.class::isInstance), | ||
| SAMPLE(Sample.class::isInstance), | ||
| SUBQUERY(Subquery.class::isInstance), | ||
| SUBQUERY(plan -> plan instanceof Subquery s && s.isView() == false), |
There was a problem hiding this comment.
I'm removing the use of Subquery in views, so we won't need to use the isView method.
There was a problem hiding this comment.
Subquery is now entirely removed from views.
| SAMPLE(Sample.class::isInstance), | ||
| SUBQUERY(Subquery.class::isInstance), | ||
| SUBQUERY(plan -> plan instanceof Subquery s && s.isView() == false), | ||
| VIEWS(plan -> false), // Views are counted in EsqlSession.gatherViewMetrics, not via plan traversal |
There was a problem hiding this comment.
Well, actually we could use plan traversal once the ViewUnionAll is implemented.
There was a problem hiding this comment.
Changed my mind. The EsqlSession.gatherViewMetrics is the way to go.
| } | ||
|
|
||
| private static boolean explicitlyExcluded(LogicalPlan plan) { | ||
| if (plan instanceof Subquery s && s.isView()) { |
There was a problem hiding this comment.
We can probably drop this too.
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml
Show resolved
Hide resolved
craigtaverner
left a comment
There was a problem hiding this comment.
I approve of the EsqlSession.gatherViewMetrics approach. But we need to revert the changes to Subquery, since that is no longer used in Views at all.
| } | ||
|
|
||
| private void gatherViewMetrics(ViewResolver.ViewResolutionResult viewResolution) { | ||
| if (metrics == null || viewResolution.viewQueries().isEmpty()) { |
There was a problem hiding this comment.
This does look like the best approach for now, since we cannot rely on Subquery.name() nor on the existence of ViewUnionAll, both of which can be empty/missing due to query simplification/compaction.
| return child(); | ||
| } | ||
|
|
||
| public String name() { |
There was a problem hiding this comment.
We should remove these changes to Subquery.
| COMPLETION(Completion.class::isInstance), | ||
| SAMPLE(Sample.class::isInstance), | ||
| SUBQUERY(Subquery.class::isInstance), | ||
| SUBQUERY(plan -> plan instanceof Subquery s && s.isView() == false), |
There was a problem hiding this comment.
Subquery is now entirely removed from views.
| SAMPLE(Sample.class::isInstance), | ||
| SUBQUERY(Subquery.class::isInstance), | ||
| SUBQUERY(plan -> plan instanceof Subquery s && s.isView() == false), | ||
| VIEWS(plan -> false), // Views are counted in EsqlSession.gatherViewMetrics, not via plan traversal |
There was a problem hiding this comment.
Changed my mind. The EsqlSession.gatherViewMetrics is the way to go.
| } | ||
|
|
||
| private static boolean explicitlyExcluded(LogicalPlan plan) { | ||
| if (plan instanceof Subquery s && s.isView()) { |
| public void testSubqueryTelemetry() { | ||
| Row innerPlan = new Row(Source.EMPTY, List.of()); | ||
|
|
||
| Subquery view = new Subquery(Source.EMPTY, innerPlan, "my_view"); |
There was a problem hiding this comment.
Subquery is no longer used for views, so this should be changed.
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml
Show resolved
Hide resolved
craigtaverner
left a comment
There was a problem hiding this comment.
LGTM, although I am wondering about the use of a plural name while all other counters use singular?
x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml
Show resolved
Hide resolved
| COMPLETION(Completion.class::isInstance), | ||
| SAMPLE(Sample.class::isInstance), | ||
| SUBQUERY(Subquery.class::isInstance), | ||
| VIEWS(plan -> false), // Views are counted in EsqlSession.gatherViewMetrics, not via plan traversal |
There was a problem hiding this comment.
Wondering about the use of plural here, while others mostly use singular.
| - set: { esql.features.completion: completion_counter } | ||
| - set: { esql.features.sample: sample_counter } | ||
| - set: { esql.features.subquery: subquery_counter } | ||
| - set: { esql.features.views: views_counter } |
There was a problem hiding this comment.
I just noticed almost all other counters use the singular, like subquery_counter, and we are almost the only one using plural. Should we switch to singular?
Adding Telemetry (stack) for views usage.
We just increment the counter every time a query uses views, ie. we don't track how many views a single query is using.
Developed using AI-assisted tooling