Add analytics plugin usage stats to _xpack/usage by imotov · Pull Request #54911 · elastic/elasticsearch

imotov · 2020-04-07T17:43:21Z

Adds analytics plugin usage stats to _xpack/usage. It looks like the usage
didn't report the actual usage in the previous versions. So, while it looks
like it is reported in 7.7, the numbers are always 0. Because code between 7.7
and 7.x/master is significantly different, I can open a separate PR for 7.7 if
we want to fix it there.

Closes #54847

Adds analytics plugin usage stats to _xpack/usage. Closes elastic#54847

elasticmachine · 2020-04-07T17:43:23Z

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

nik9000 · 2020-04-07T17:54:36Z

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

            ResourceWatcherService resourceWatcherService, ScriptService scriptService, NamedXContentRegistry xContentRegistry,
            Environment environment, NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry,
            IndexNameExpressionResolver indexNameExpressionResolver) {
-        return singletonList(new AnalyticsUsage());


nik9000 · 2020-04-07T17:59:59Z

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

 import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.common.xcontent.ContextParser;
 import org.elasticsearch.xpack.core.analytics.action.AnalyticsStatsAction;
+import org.elasticsearch.xpack.core.watcher.common.stats.Counters;


I feel like we might be better off using an EnumMap<AnalyticsStatsAction.Item, AtomicLong> than this string based thing. Serialization would have to be a bit more explicit, but I don't mind that too much.

On the other hand, Counters is nice from a BWC perspective. Maybe just move it out of the watcher package in a follow up change? Maybe drop the enum entirely and just us a string?

I have replaced Counters with Enum-based Counters, which for now resides in the analytics package, but we can move it to a more generic package if we decide to start using them for other areas.

…based counters

imotov · 2020-04-08T20:00:43Z

@polyfractal @nik9000 I implemented some changes. I would love to hear what you think about it.

nik9000 · 2020-04-08T21:32:01Z

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

+                vals[i] = val;
+            }
+        }
+        counters = new AtomicLongArray(vals);


So, like, we have to add enum entries to the end and they'll come through as 0 if they aren't set. Right?

I think it is worth adding comments about that, both to the enum class and to this one.

This is basically to handle the situation when an older node have less stat categories or a newer node has more stats categories. I added a comment to the enum but I agree I should duplicate it here.

nik9000 · 2020-04-08T21:34:30Z

x-pack/plugin/core/src/test/java/org/elasticsearch/analytics/EnumCountersTests.java

+        counters.inc(TestV1.A, 1);
+        counters.inc(TestV1.B, 2);
+        counters.inc(TestV1.C, 3);
+        EnumCounters<TestV2> newCounters = serialize(counters, in -> new EnumCounters<>(in, TestV2.class));


Nice trick!

…ics-xpack-usage

imotov · 2020-04-10T13:12:19Z

@polyfractal @nik9000 I have added comments that @nik9000 has requested.

nik9000

LGTM

polyfractal

Left a few minor comments and a serialization question, but otherwise LGTM! Thanks for fixing <3

polyfractal · 2020-04-13T14:11:35Z

...cs/src/main/java/org/elasticsearch/xpack/analytics/action/AnalyticsUsageTransportAction.java

    @Override
    protected void masterOperation(Task task, XPackUsageRequest request, ClusterState state,
                                   ActionListener<XPackUsageFeatureResponse> listener) {
        boolean available = licenseState.isDataScienceAllowed();


Oof, just noticed "DataScience"... we should probably change that at some point 😓 Not for this PR though :)

polyfractal · 2020-04-13T14:13:19Z

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

+                vals[i] = val;
+            }
+        }
+        counters = new AtomicLongArray(vals);


polyfractal · 2020-04-13T14:17:21Z

...n/core/src/main/java/org/elasticsearch/xpack/core/analytics/action/AnalyticsStatsAction.java

-                ttestUsage = 0;
+                counters = new EnumCounters<>(Item.class);
+                if (in.getVersion().onOrAfter(Version.V_7_7_0)) {
+                    counters.inc(Item.BOXPLOT, in.readLong());


I think this should be a readVLong() instead of readLong()?

Good catch!

polyfractal · 2020-04-13T14:20:22Z

...n/core/src/main/java/org/elasticsearch/xpack/core/analytics/action/AnalyticsStatsAction.java

    }

+    /**
+     * Items to track. Serialized by ordinals. Append only, don't remove or change order of items in this list.


Should we have a test somewhere that enforces this? E.g. enums that extend writeable directly can implement AbstractWriteableEnumTestCase. Not the same situation here, but having something conceptually similar to testValidOrdinals() would be nice. Just verifies that BOXPLOT == 0, etc etc

Indeed we need a bwc test for usage. Right now it is completely broken so it didn't make sense to have it. But as soon as I back port this I will open a PR for bwc test for usage.

Adds analytics plugin usage stats to _xpack/usage. Closes elastic#54847

Adds analytics plugin usage stats to _xpack/usage. Closes #54847

Add analytics plugin usage stats to _xpack/usage

b86bd57

Adds analytics plugin usage stats to _xpack/usage. Closes elastic#54847

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

imotov requested review from nik9000 and polyfractal April 7, 2020 17:43

nik9000 reviewed Apr 7, 2020

View reviewed changes

imotov added 2 commits April 7, 2020 16:41

Address review comments by replacing String based counters with enum …

0e1bb69

…based counters

Fix usage docs

c6818b9

imotov requested a review from nik9000 April 8, 2020 21:34

nik9000 reviewed Apr 8, 2020

View reviewed changes

imotov added 2 commits April 10, 2020 09:08

Add comment about underlying EnumCounters assumptions

c22680e

Merge remote-tracking branch 'elastic/master' into issue-54847-analyt…

67eb9fa

…ics-xpack-usage

Fix checkstyle

684cbc3

nik9000 approved these changes Apr 10, 2020

View reviewed changes

$polyfractal$

polyfractal approved these changes Apr 13, 2020

View reviewed changes

imotov added 2 commits April 13, 2020 10:33

Fix boxplot stats serialization

7f5e89e

Add node response tests

950be0f

imotov merged commit e593f3e into elastic:master Apr 13, 2020

imotov added the backport pending label Apr 13, 2020

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

Add analytics plugin usage stats to _xpack/usage (elastic#54911)

9c487ad

Adds analytics plugin usage stats to _xpack/usage. Closes elastic#54847

imotov mentioned this pull request Apr 14, 2020

[7.x] Add analytics plugin usage stats to _xpack/usage (#54911) #55162

Merged

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

[7.x] Add analytics plugin usage stats to _xpack/usage (#54911) (#55162)

1754e50

Adds analytics plugin usage stats to _xpack/usage. Closes #54847

imotov removed the backport pending label Apr 17, 2020

imotov deleted the issue-54847-analytics-xpack-usage branch May 1, 2020 22:22

russcam mentioned this pull request May 29, 2020

7.8.0 Meta ticket elastic/elasticsearch-net#4718

Closed

17 tasks

DaveCTurner mentioned this pull request Aug 18, 2020

Mixed-version serialization bug in cluster:monitor/xpack/usage responses #61279

Closed

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

Conversation

imotov commented Apr 7, 2020

Uh oh!

elasticmachine commented Apr 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Apr 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imotov commented Apr 10, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

polyfractal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

$@polyfractal$ polyfractal left a comment