Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/143745.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
area: ES|QL
issues: []
pr: 143745
summary: Add (stack) telemetry for views
type: enhancement

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering, do we add changelog entries for unreleased (snapshot-ony) features? Perhaps the label should be >non-issue?

Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,8 @@ public String nodeString(NodeStringFormat format) {
public LogicalPlan plan() {
return child();
}

public String name() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It reminds me of this PR, it adds ViewUnionAll that extends UnionAll. I wonder if it affects view and subquery's telemetry?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR merged, and so Subquery no longer has names.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove these changes to Subquery.

return name;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@
import org.elasticsearch.xpack.esql.planner.premapper.PreMapper;
import org.elasticsearch.xpack.esql.plugin.ComputeService;
import org.elasticsearch.xpack.esql.plugin.TransportActionServices;
import org.elasticsearch.xpack.esql.telemetry.FeatureMetric;
import org.elasticsearch.xpack.esql.telemetry.Metrics;
import org.elasticsearch.xpack.esql.telemetry.PlanTelemetry;
import org.elasticsearch.xpack.esql.view.ViewResolver;
Expand Down Expand Up @@ -273,6 +274,7 @@ private void analyseAndExecute(
ActionListener<Versioned<Result>> listener
) {
assert ThreadPool.assertCurrentThreadPool(ThreadPool.Names.SEARCH);
gatherViewMetrics(viewResolution);
PlanTimeProfile planTimeProfile = request.profile() ? new PlanTimeProfile() : null;

ZoneId timeZone = request.timeZone() == null
Expand Down Expand Up @@ -719,6 +721,14 @@ private void gatherSettingsMetrics(EsqlStatement statement) {
statement.settings().stream().map(QuerySetting::name).distinct().forEach(metrics::incSetting);
}

private void gatherViewMetrics(ViewResolver.ViewResolutionResult viewResolution) {
if (metrics == null || viewResolution.viewQueries().isEmpty()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}
// If any views were used, increment the views metric

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this comment helps the readability of the code. I think it is obvious what the next line does :-).

metrics.inc(FeatureMetric.VIEWS);
}

/**
* Associates errors that occurred during field-caps with the cluster info in the execution info.
* - Skips clusters that are no longer running, as they have already been marked as successful, skipped, or failed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ public enum FeatureMetric {
FUSE(Fuse.class::isInstance),
COMPLETION(Completion.class::isInstance),
SAMPLE(Sample.class::isInstance),
SUBQUERY(Subquery.class::isInstance),
SUBQUERY(plan -> plan instanceof Subquery s && s.name() == null),
VIEWS(plan -> false), // Views are counted in EsqlSession.gatherViewMetrics, not via plan traversal

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, actually we could use plan traversal once the ViewUnionAll is implemented.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed my mind. The EsqlSession.gatherViewMetrics is the way to go.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering about the use of plural here, while others mostly use singular.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks!

MMR(MMR.class::isInstance),
PROMQL(PromqlCommand.class::isInstance),
URI_PARTS(UriParts.class::isInstance),
Expand Down Expand Up @@ -129,6 +130,10 @@ public static void set(LogicalPlan plan, BitSet bitset) {
}

private static boolean explicitlyExcluded(LogicalPlan plan) {
// Named Subqueries represent views, which are counted separately in EsqlSession.gatherViewMetrics
if (plan instanceof Subquery s && s.name() != null) {
return true;
}
return excluded.stream().anyMatch(x -> x.isInstance(plan));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@
import org.elasticsearch.xpack.core.watcher.common.stats.Counters;
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
import org.elasticsearch.xpack.esql.analysis.Verifier;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
import org.elasticsearch.xpack.esql.expression.function.FunctionDefinition;
import org.elasticsearch.xpack.esql.index.IndexResolution;
import org.elasticsearch.xpack.esql.plan.logical.Row;
import org.elasticsearch.xpack.esql.plan.logical.Subquery;

import java.util.BitSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -47,6 +51,7 @@
import static org.elasticsearch.xpack.esql.telemetry.FeatureMetric.SORT;
import static org.elasticsearch.xpack.esql.telemetry.FeatureMetric.STATS;
import static org.elasticsearch.xpack.esql.telemetry.FeatureMetric.TS;
import static org.elasticsearch.xpack.esql.telemetry.FeatureMetric.VIEWS;
import static org.elasticsearch.xpack.esql.telemetry.FeatureMetric.WHERE;
import static org.elasticsearch.xpack.esql.telemetry.Metrics.FEATURES_PREFIX;
import static org.elasticsearch.xpack.esql.telemetry.Metrics.FUNC_PREFIX;
Expand Down Expand Up @@ -839,6 +844,26 @@ public void testPromql() {
assertEquals(0, subqueryInFromCommand(c));
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho, this method also has too many comments.

// Views are counted separately via EsqlSession.gatherViewMetrics (ad-hoc approach)
Subquery view = new Subquery(Source.EMPTY, innerPlan, "my_view");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subquery is no longer used for views, so this should be changed.

BitSet viewBitSet = new BitSet();
FeatureMetric.set(view, viewBitSet);
// Named Subqueries are excluded, so no metrics should be set
assertFalse("VIEWS should not be set for named Subquery (counted ad-hoc)", viewBitSet.get(VIEWS.ordinal()));
assertFalse("SUBQUERY should not be set for named Subquery", viewBitSet.get(FeatureMetric.SUBQUERY.ordinal()));

// An unnamed Subquery represents an inline subquery (not a view)
Subquery subquery = new Subquery(Source.EMPTY, innerPlan);
BitSet subqueryBitSet = new BitSet();
FeatureMetric.set(subquery, subqueryBitSet);
assertFalse("VIEWS should not be set for unnamed Subquery", subqueryBitSet.get(VIEWS.ordinal()));
assertTrue("SUBQUERY should be set for unnamed Subquery", subqueryBitSet.get(FeatureMetric.SUBQUERY.ordinal()));
}

private long dissect(Counters c) {
return c.get(FEATURES_PREFIX + DISSECT);
}
Expand Down Expand Up @@ -923,6 +948,10 @@ private long subqueryInFromCommand(Counters c) {
return c.get(FEATURES_PREFIX + FeatureMetric.SUBQUERY);
}

private long views(Counters c) {
Comment thread
luigidellaquila marked this conversation as resolved.
Outdated
return c.get(FEATURES_PREFIX + VIEWS);
}

private long function(String function, Counters c) {
return c.get(FUNC_PREFIX + function);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ setup:
- set: { esql.features.completion: completion_counter }
- set: { esql.features.sample: sample_counter }
- set: { esql.features.subquery: subquery_counter }
- set: { esql.features.views: views_counter }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

- set: { esql.features.uri_parts: uri_parts_counter }
- set: { esql.features.registered_domain: registered_domain_counter }
- set: { esql.features.mmr: mmr_counter }
Expand Down Expand Up @@ -402,3 +403,40 @@ took:
- exists: esql.took.lt_10h
- exists: esql.took.lt_1d
- exists: esql.took.gt_1d

---
"Views telemetry (snapshot only)":
- requires:
test_runner_features: [ capabilities ]
capabilities:
- method: POST
path: /_query
parameters: [ ]
capabilities: [ snapshot_test_for_telemetry_v2 ]
- method: GET
path: /_query/view
capabilities: [ view_index_abstraction ]
Comment thread
craigtaverner marked this conversation as resolved.
reason: "Views telemetry test requires snapshot build with views enabled"

- do: { xpack.usage: { } }
- set: { esql.features.views: views_counter }
- set: { esql.features.subquery: subquery_counter }

- do:
esql.put_view:
name: test_view_for_telemetry
body:
query: 'FROM test | WHERE data > 0'

- do:
esql.query:
body:
query: 'FROM test_view_for_telemetry | LIMIT 10'

- do: { xpack.usage: { } }
- gt: { esql.features.views: $views_counter }
- match: { esql.features.subquery: $subquery_counter }

- do:
esql.delete_view:
name: test_view_for_telemetry
Loading