ESQL: Implement a MetricsAware interface #120527

bpintea · 2025-01-21T12:48:37Z

This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics.

Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur).

The functions counting now also validates that behind a name there is actually a function registered.

Closes #115992.

This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur).

elasticsearchmachine · 2025-01-21T12:49:01Z

Hi @bpintea, I've created a changelog YAML for you.

elasticsearchmachine · 2025-01-21T15:41:29Z

Hi @bpintea, I've updated the changelog YAML for you.

elasticsearchmachine · 2025-01-21T18:05:02Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

costin

LGTM. My main comments are about using the Telemetry name instead of metrics and some small cleanups here and there; nothing major.

costin · 2025-01-22T02:50:55Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/stats/PlanningMetrics.java

@@ -7,28 +7,56 @@

 package org.elasticsearch.xpack.esql.stats;


o.e.x.esql.telemetry

Created a new package and moved all telemetry-related classes there.

costin · 2025-01-22T02:51:49Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/stats/PlanningMetrics.java

 import java.util.Locale;
 import java.util.Map;
+import java.util.Set;
+
+import static org.elasticsearch.common.Strings.format;

 /**
 * This class is responsible for collecting metrics related to ES|QL planning.
 */
 public class PlanningMetrics {


There's not much related to planning here so I propose renaming to PlanTelemetry and update the javadoc accordingly.

Updated (initial naming was taken from the proposed design doc).

costin · 2025-01-22T02:57:16Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/stats/PlanningMetrics.java

+        var functionName = functionRegistry.resolveAlias(name);
+        if (functionRegistry.functionExists(functionName)) {
+            // The metrics have been collected initially with their uppercase spelling
+            add(functions, functionName.toUpperCase(Locale.ROOT));


Is there a way to remove the toUpper() repetition in this class and EsqlFunctionRegistry?

costin · 2025-01-22T02:58:52Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

@@ -218,7 +218,7 @@ private LogicalPlan resolveIndex(UnresolvedRelation plan, IndexResolution indexR
                        plan.metadataFields(),
                        plan.indexMode(),
                        indexResolutionMessage,
-                        plan.commandName()
+                        plan.metricName()


Since the string is provided internally, is there still a need to specify it in the constructor?

Yes, because in some cases (like with METRICS) we don't have a dedicated container.

costin · 2025-01-22T02:59:05Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/capabilities/MetricsAware.java

+/**
+ * Interface for plan nodes that need to be accounted in the statistics
+ */
+public interface MetricsAware {


TelemetryAware

costin · 2025-01-22T03:02:20Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlParser.java

    public LogicalPlan createStatement(String query, QueryParams params) {
+        return createStatement(query, params, new PlanningMetrics(new EsqlFunctionRegistry()));


Where is this used - I'd rather force consumer to change the constructor instead of hiding this object creation underneath.

It's only used in tests (I'd added a comment).
This could be moved out, but subsequently, as there are plenty of tests not caring about params or metrics.

costin · 2025-01-22T03:03:46Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/ExpressionBuilder.java

@@ -116,9 +117,11 @@ public abstract class ExpressionBuilder extends IdentifierBuilder {
    public static final int MAX_EXPRESSION_DEPTH = 400;

    protected final QueryParams params;
+    private final PlanningMetrics metrics;


Make it protected so subclasses can refer to it instead of having their own copies.
To avoid future disruption in the constructor signature, introduce a basic ParsingContext record (record ParsingContext(QueryParams params, PlanTelemetry) and use that instead.

Ah, right, this was silly. Thx.

costin · 2025-01-22T03:05:14Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

@@ -87,19 +89,25 @@ public class LogicalPlanBuilder extends ExpressionBuilder {

    interface PlanFactory extends Function<LogicalPlan, LogicalPlan> {}

+    private final PlanningMetrics metrics;


See my comment on ExpressionBuilder.

costin · 2025-01-22T03:05:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

@@ -481,7 +489,7 @@ public LogicalPlan visitMetricsCommand(EsqlBaseParser.MetricsCommandContext ctx)
            List.of(new MetadataAttribute(source, MetadataAttribute.TSID_FIELD, DataType.KEYWORD, false)),
            IndexMode.TIME_SERIES,
            null,
-            "FROM TS"
+            null


Remove the constructor is the label is not needed anymore.

Added a new c'tor to not take that last parameter.

costin · 2025-01-22T03:06:00Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

@@ -530,7 +538,7 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {
            emptyList(),
            IndexMode.LOOKUP,
            null,
-            "???"
+            null // should not end up being counted in the metrics


Same as above.

elasticsearchmachine · 2025-01-27T18:26:34Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 120527

This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur). The functions counting now also validates that behind a name there is actually a function registered. Closes elastic#115992. (cherry picked from commit a4482d4)

This reverts commit a4482d4.

This reverts commit a4482d4. It turns out that `PlanTelemetry` can add quite a bit of memory usage, at least on "rude" queries. In `HeapAttackIT.testHugeManyConcat`, this was using 30MB. I'd like to revert this to see if we can - either reduce its memory footprint or - track its memory somehow.

This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur). The functions counting now also validates that behind a name there is actually a function registered. Closes elastic#115992. (cherry picked from commit a4482d4)

* ESQL: Implement a MetricsAware interface (#120527) This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur). The functions counting now also validates that behind a name there is actually a function registered. Closes #115992. (cherry picked from commit a4482d4) * Drop the HashSet gating when counting commands The telemetry accounting is no longer done in just one place in the parser, but split, so that no HashSet is required to discard duplicate accounting of the same node. This lowers the memory requirements.

* ESQL: Implement a MetricsAware interface (elastic#120527) This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur). The functions counting now also validates that behind a name there is actually a function registered. Closes elastic#115992. (cherry picked from commit a4482d4) * Drop the HashSet gating when counting commands The telemetry accounting is no longer done in just one place in the parser, but split, so that no HashSet is required to discard duplicate accounting of the same node. This lowers the memory requirements.

* ESQL: Implement a MetricsAware interface (#120527) This implements an interface that export the names of the plan nodes and functions that need to be counted in the metrics. Also, the metrics are now counted from within the parser. This should allow correct accounting for the cases where some nodes can appear both standalone or part other nodes' children (like Aggregate being a child of INLINESTATS, so no STATS counting should occur). The functions counting now also validates that behind a name there is actually a function registered. Closes #115992. (cherry picked from commit a4482d4) * Drop the HashSet gating when counting commands The telemetry accounting is no longer done in just one place in the parser, but split, so that no HashSet is required to discard duplicate accounting of the same node. This lowers the memory requirements.

bpintea added 2 commits January 21, 2025 13:38

spotless

b99ae35

bpintea added >enhancement auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 21, 2025

Update docs/changelog/120527.yaml

f0af22c

bpintea mentioned this pull request Jan 21, 2025

ESQL: define interfaces as contractual extension points #119856

Open

6 tasks

bpintea and others added 3 commits January 21, 2025 16:29

Fix tests setup

2d74a9c

Merge branch 'main' into enh/metrics_aware

27d5613

Update docs/changelog/120527.yaml

9caadc2

bpintea marked this pull request as ready for review January 21, 2025 18:04

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 21, 2025

costin approved these changes Jan 22, 2025

View reviewed changes

bpintea and others added 4 commits January 24, 2025 11:33

Address review comments

4ef734b

[CI] Auto commit changes from spotless

9d546be

Merge branch 'main' into enh/metrics_aware

d4f8c68

Merge branch 'main' into enh/metrics_aware

99d8782

bpintea added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jan 27, 2025

elasticsearchmachine merged commit a4482d4 into elastic:main Jan 27, 2025
16 checks passed

bpintea deleted the enh/metrics_aware branch January 27, 2025 18:25

elasticsearchmachine added the backport pending label Jan 27, 2025

alex-spies added a commit to alex-spies/elasticsearch that referenced this pull request Jan 28, 2025

Revert "ESQL: Implement a MetricsAware interface (elastic#120527)"

a0b18d8

This reverts commit a4482d4.

bpintea mentioned this pull request Jan 28, 2025

[8.x] ESQL: Implement a MetricsAware interface (#120527) #120989

Closed

bpintea removed the backport pending label Jan 28, 2025

bpintea mentioned this pull request Jan 28, 2025

ESQL: Implement a MetricsAware interface #121074

Merged

nik9000 mentioned this pull request Jan 29, 2025

[CI] HeapAttackIT class failing #121112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Implement a MetricsAware interface #120527

ESQL: Implement a MetricsAware interface #120527

bpintea commented Jan 21, 2025 •

edited

Loading

elasticsearchmachine commented Jan 21, 2025

elasticsearchmachine commented Jan 21, 2025

elasticsearchmachine commented Jan 21, 2025

costin left a comment

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

bpintea Jan 27, 2025

costin Jan 22, 2025

elasticsearchmachine commented Jan 27, 2025

		@@ -7,28 +7,56 @@

		package org.elasticsearch.xpack.esql.stats;

		public LogicalPlan createStatement(String query, QueryParams params) {
		return createStatement(query, params, new PlanningMetrics(new EsqlFunctionRegistry()));

		@@ -87,19 +89,25 @@ public class LogicalPlanBuilder extends ExpressionBuilder {

		interface PlanFactory extends Function<LogicalPlan, LogicalPlan> {}

		private final PlanningMetrics metrics;

ESQL: Implement a MetricsAware interface #120527

ESQL: Implement a MetricsAware interface #120527

Conversation

bpintea commented Jan 21, 2025 • edited Loading

elasticsearchmachine commented Jan 21, 2025

elasticsearchmachine commented Jan 21, 2025

elasticsearchmachine commented Jan 21, 2025

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 27, 2025

💔 Backport failed

bpintea commented Jan 21, 2025 •

edited

Loading