Expose agg usage in Feature Usage API #55732

imotov · 2020-04-24T15:17:58Z

Counts usage of the aggs and exposes them on the /_nodes/usage.

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes elastic#53746

elasticmachine · 2020-04-24T15:18:00Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

not-napoleon · 2020-04-24T15:42:52Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java

+     * Returns the name of the Values Source Type for stats purposes
+     * @return the name of the Values Source Type
+     */
+    String value();


Maybe typeName() instead? we're already using value for a lot of things in aggregations support.

not-napoleon · 2020-04-24T15:52:29Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

+        }
+    }
+
+    private static class MultiRegistryEntry extends RegistryEntry {


I don't think Multi should be it's own case. Instead, we can loop in the list version of register and just create multiple SingleRegistryEntry records. That way, report usage can still track to a single ValuesSourceType, and the only "special case" is around Any, which we're planning to remove.

not-napoleon · 2020-04-24T16:15:00Z

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

         * @param aggregatorSupplier An Aggregation-specific specialization of AggregatorSupplier which will construct the mapped aggregator
         *                           from the aggregation standard set of parameters.
         */
        public void registerAny(String aggregationName, AggregatorSupplier aggregatorSupplier) {
-            register(aggregationName, (ignored) -> true, aggregatorSupplier);
+            register(aggregationName, new AnyRegistryEntry(aggregatorSupplier));


So, @csoulios is working on a near term plan to get rid of the literal any, and replace registerAny with just registering all the CoreValuesSourceTypes. We could adopt something like that here, and just register an explicit list of SingleRegistryEntry cases instead. Or leave this as is and change it over when the rest of Christos's ANY work merges.

Just pushed PR #55747 that removes registerAny() method

imotov · 2020-04-24T17:20:00Z

After talking to @polyfractal I think I might need to redo this one. I feel like placement might have been wrong. We have aggregations that have 0, 1 or 2 ValuesSources, and this is not going away. So, I probably need to move this logic somewhere higher.

…sage

imotov · 2020-04-28T16:00:50Z

@polyfractal @not-napoleon I think it is ready for review.

…sage

polyfractal

Left some comments, mainly around the periphery and with javadocs/comments/etc. I looked over the changes to the VSRegistry and they look fine to me, but I'll defer to @not-napoleon on the final say for that.

polyfractal · 2020-04-28T17:39:33Z

...ution/archives/integ-test-zip/src/test/java/org/elasticsearch/test/rest/NodeRestUsageIT.java

+            final Request index = new Request("POST", "/test/_doc/" + i);
+            index.setJsonEntity("{\"str\": \"val\", \"foo\":\"bar\", \"num\": 5, \"start\": \"2020-03-15\"}");
+            client().performRequest(index);
+        }


Should we refresh after indexing? Or does it not matter for the usage stats if the fields are potentially unmapped?

If it doesn' matter, maybe we could skip indexing to save some time?

polyfractal · 2020-04-28T17:41:27Z

docs/reference/cluster/nodes-usage.asciidoc

-        "org.elasticsearch.rest.action.document.RestGetAction": 1,
-        "org.elasticsearch.rest.action.search.RestSearchAction": 19, <3>
-        "org.elasticsearch.rest.action.admin.cluster.RestNodesInfoAction": 36
+        "nodes_usage_action": 1,


Was this name change due to the PR, or were the docs always incorrect?

No, we ignore this section. So when it was refactored in May 2017 we didn't update the docs.

polyfractal · 2020-04-28T17:48:16Z

server/src/main/java/org/elasticsearch/search/SearchModule.java

@@ -483,6 +488,8 @@ private void registerAggregation(AggregationSpec spec, ValuesSourceRegistry.Buil
        Consumer<ValuesSourceRegistry.Builder> register = spec.getAggregatorRegistrar();
        if (register != null) {
            register.accept(builder);
+        } else {
+            usageService.registerAggregationUsage(spec.getName().getPreferredName());


Could we add a comment here, to the effect of the registry handles usage tracking, but since not all aggs use it we need to track the other ones explicitly? I feel like I'll stumble on this in the future and be confused :)

Hmm, I was 100% I had this comment there. Strange.

polyfractal · 2020-04-28T17:55:37Z

server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java

@@ -238,4 +240,7 @@ protected static Aggregator asMultiBucketAggregator(final AggregatorFactory fact
        return new MultiBucketAggregatorWrapper(bigArrays, searchContext, parent, factory, first);
    }

+    public String getStatsSubtype() {


Could we add a javadoc for what this is/used for?

polyfractal · 2020-04-28T18:04:01Z

server/src/main/java/org/elasticsearch/search/SearchModule.java


        registerAggregation(new AggregationSpec(AvgAggregationBuilder.NAME, AvgAggregationBuilder::new, AvgAggregationBuilder.PARSER)
            .addResultReader(InternalAvg::new)
            .setAggregatorRegistrar(AvgAggregationBuilder::registerAggregators), builder);
        registerAggregation(new AggregationSpec(WeightedAvgAggregationBuilder.NAME, WeightedAvgAggregationBuilder::new,
-            WeightedAvgAggregationBuilder.PARSER).addResultReader(InternalWeightedAvg::new), builder);
+            WeightedAvgAggregationBuilder.PARSER).addResultReader(InternalWeightedAvg::new)
+            .setAggregatorRegistrar(WeightedAvgAggregationBuilder::registerUsage), builder);


Heh, this is a bit cheeky...took me a minute to figure out what was going on here :)

polyfractal · 2020-04-28T18:06:45Z

server/src/main/java/org/elasticsearch/usage/UsageService.java

+
+    public void incAggregationUsage(String aggregationName, String valuesSourceType) {
+        Map<String, LongAdder> valuesSourceMap = aggs.get(aggregationName);
+        // Not all aggs register their usage at the moment we also don't register them in test context


Are there other aggs that aren't caught by the else in SearchModule (using registerAggregationUsage(String aggregationName) )?

Hmm, good point. Let's find out.

polyfractal · 2020-04-28T18:08:51Z

server/src/main/java/org/elasticsearch/usage/UsageService.java

+            if (adder != null) {
+                adder.increment();
+            } else {
+                throw new IllegalArgumentException("Unknown subtype [" + aggregationName + "][" + valuesSourceType + "]");


I wonder if we should just log an error but not actually throw an exception? I'm thinking if we end up breaking something on the usage side, it'd be a shame if it actually broke the aggs until we got a bugfix out

That helped me to find a lot of strange behaving aggregations. But you are right, I might just replace this with assert for now.

polyfractal · 2020-04-28T18:11:19Z

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/AnalyticsPlugin.java

@@ -105,6 +105,7 @@ public AnalyticsPlugin() { }
                TTestAggregationBuilder::new,
                usage.track(AnalyticsStatsAction.Item.T_TEST, checkLicense(TTestAggregationBuilder.PARSER)))


Hmm... we should probably discuss if we still want these being tracked separately? Unrelated to this PR though, just something I hadn't thought about :)

…sage

imotov · 2020-04-30T11:31:37Z

@elasticmachine update branch

Suppresses bwc tests for backport of elastic#55732

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes elastic#53746

Suppresses bwc tests for backport of #55732

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes #53746

Updates the serialization version and re-enables bwc tests for nodes usage api after backport of elastic#55732

Updates the serialization version and re-enables bwc tests for nodes usage api after backport of #55732

Relates: #4718, elastic/elasticsearch#55732

Relates: #4718, elastic/elasticsearch#55732 Co-authored-by: Russ Cam <[email protected]>

Expose agg usage in Feature Usage API

c0a7dd4

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes elastic#53746

imotov added >enhancement :Analytics/Aggregations Aggregations v8.0.0 v7.8.0 labels Apr 24, 2020

imotov requested review from not-napoleon and polyfractal April 24, 2020 15:17

imotov marked this pull request as draft April 24, 2020 15:29

not-napoleon requested changes Apr 24, 2020

View reviewed changes

imotov added 9 commits April 24, 2020 17:06

Refactor to include non value sources aggregations

1cc437c

Fix reported values source type for parent and children aggs

9c8510a

Merge remote-tracking branch 'elastic/master' into issue-53746-aggs-u…

f4ab1e7

…sage

Refactor SearchModule constructor

6207497

Fix subtype in TTest and IPRanges

b8e7051

Fix more subtypes in aggs that don't register themselves

530a5dd

Fix doc tests

39a9bb3

Fix docs

980ca33

Fix ScriptedMetricAggregatorTests

2eae809

imotov marked this pull request as ready for review April 27, 2020 18:26

imotov requested a review from not-napoleon April 27, 2020 18:26

imotov added 3 commits April 28, 2020 10:48

Merge remote-tracking branch 'elastic/master' into issue-53746-aggs-u…

5c0393a

…sage

Fix compilation issues after merge

4c6b4dd

Fix merge fallout

4a76f01

imotov added 2 commits April 28, 2020 12:32

Merge remote-tracking branch 'elastic/master' into issue-53746-aggs-u…

2d0e952

…sage

This gets stale quickly...

424884a

$polyfractal$

polyfractal approved these changes Apr 28, 2020

View reviewed changes

Address review comments

ca2fb25

imotov added 2 commits April 29, 2020 17:38

Address review comments

6f0c4fd

Merge remote-tracking branch 'elastic/master' into issue-53746-aggs-u…

439aada

…sage

Merge branch 'master' into issue-53746-aggs-usage

b169633

imotov merged commit b909cee into elastic:master Apr 30, 2020

imotov added the backport pending label Apr 30, 2020

imotov added a commit to imotov/elasticsearch that referenced this pull request Apr 30, 2020

Suppress nodes usage tests for backport

62303f9

Suppresses bwc tests for backport of elastic#55732

imotov mentioned this pull request Apr 30, 2020

Disables nodes usage tests for backport #56046

Merged

imotov added a commit to imotov/elasticsearch that referenced this pull request Apr 30, 2020

Expose agg usage in Feature Usage API (elastic#55732)

d0fdc5c

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes elastic#53746

imotov added a commit that referenced this pull request Apr 30, 2020

Suppress nodes usage tests for backport (#56046)

0c0c289

Suppresses bwc tests for backport of #55732

imotov added a commit that referenced this pull request Apr 30, 2020

Expose agg usage in Feature Usage API (#55732) (#56048)

d8f9df7

Counts usage of the aggs and exposes them on the _nodes/usage/. Closes #53746

imotov added a commit to imotov/elasticsearch that referenced this pull request Apr 30, 2020

Re-enable nodes usage tests and update serialization version

94c8f88

Updates the serialization version and re-enables bwc tests for nodes usage api after backport of elastic#55732

imotov mentioned this pull request Apr 30, 2020

Re-enable nodes usage tests and update serialization version #56051

Merged

imotov added a commit that referenced this pull request Apr 30, 2020

Re-enable nodes usage tests and update serialization version (#56051)

1c80e85

Updates the serialization version and re-enables bwc tests for nodes usage api after backport of #55732

imotov removed the backport pending label Apr 30, 2020

imotov deleted the issue-53746-aggs-usage branch May 1, 2020 22:22

giladgal added the release highlight label May 7, 2020

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 4, 2020

Add aggregation usage to nodes usage

1223bd1

Relates: #4718, elastic/elasticsearch#55732

russcam mentioned this pull request Jun 4, 2020

Add aggregation usage to nodes usage elastic/elasticsearch-net#4737

Merged

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020

Add aggregation usage to nodes usage (#4737)

e9d54e0

Relates: #4718, elastic/elasticsearch#55732

github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020

Add aggregation usage to nodes usage (#4737)

76e1d78

Relates: #4718, elastic/elasticsearch#55732

github-actions bot pushed a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020

Add aggregation usage to nodes usage (#4737)

cce1b3a

Relates: #4718, elastic/elasticsearch#55732

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020

Add aggregation usage to nodes usage (#4737) (#4757)

f41d38d

Relates: #4718, elastic/elasticsearch#55732 Co-authored-by: Russ Cam <[email protected]>

russcam added a commit to elastic/elasticsearch-net that referenced this pull request Jun 10, 2020

Add aggregation usage to nodes usage (#4737) (#4758)

d969dfe

Relates: #4718, elastic/elasticsearch#55732 Co-authored-by: Russ Cam <[email protected]>

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose agg usage in Feature Usage API #55732

Expose agg usage in Feature Usage API #55732

imotov commented Apr 24, 2020

elasticmachine commented Apr 24, 2020

not-napoleon Apr 24, 2020

not-napoleon Apr 24, 2020

not-napoleon Apr 24, 2020

csoulios Apr 24, 2020 •

edited

Loading

imotov commented Apr 24, 2020

imotov commented Apr 28, 2020

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

imotov Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

imotov Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

imotov Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

imotov Apr 28, 2020

$@polyfractal$ polyfractal Apr 28, 2020

imotov commented Apr 30, 2020

		@@ -105,6 +105,7 @@ public AnalyticsPlugin() { }
		TTestAggregationBuilder::new,
		usage.track(AnalyticsStatsAction.Item.T_TEST, checkLicense(TTestAggregationBuilder.PARSER)))

Expose agg usage in Feature Usage API #55732

Expose agg usage in Feature Usage API #55732

Conversation

imotov commented Apr 24, 2020

elasticmachine commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

imotov commented Apr 24, 2020

imotov commented Apr 28, 2020

polyfractal 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

imotov commented Apr 30, 2020

csoulios Apr 24, 2020 •

edited

Loading

$@polyfractal$ polyfractal left a comment