Stats to record how often the ClusterState diff mechanism is used successfully #26973

DaveCTurner · 2017-10-11T16:51:33Z

It's believed that using diffs obsoletes the other mechanism for reusing the
bits of the ClusterState that didn't change between updates, but in fact we
don't know for sure how often the diff mechanism works successfully. The stats
collected here will tell us.

PR opened for comments on approach, and because I'm about to get on a plane so it's either now or much later. I think I'd like to add stats on how often these things are received as well as sent in a similar fashion.

… used successfully. It's believed that using diffs obsoletes the other mechanism for reusing the bits of the ClusterState that didn't change between updates, but in fact we don't know for sure how often the diff mechanism works successfully. The stats collected here will tell us.

bleskes

Code wise, I left some minor comments. Looks good otherwise.

I have some higher level comments:

This is currently implemented on the master side. I think it will have more value on the node side - we can see that if a node is struggling it falls back more to full cluster state publishing etc.
We need a unit test.
Can you also add a rest test - this will allow use to check we do the right thing in a mixed cluster version (if it's above the right version).
think this can go into 6.1.

bleskes · 2017-10-11T17:17:12Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

@@ -90,6 +94,10 @@
    private final IncomingClusterStateListener incomingClusterStateListener;
    private final DiscoverySettings discoverySettings;

+    private long fullClusterStateSentCount = 0;


these should probably be atomic...

It's sneaky. 😇

bleskes · 2017-10-11T17:24:31Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+/**
+ * Class encapsulating stats about the PublishClusterStateAction
+ */
+public class PublishClusterStateStats implements Writeable, ToXContentFragment {


I think you want ToXContentObject here.

bleskes · 2017-10-11T17:25:25Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+    private final long clusterStateDiffSentCount;
+    private final long incompatibleClusterStateDiffVersionCount;
+
+    public PublishClusterStateStats(long fullClusterStateSentCount,


can you java doc the parameters?

Params or getters? Or both?

jasontedor

I agree with @bleskes comments, I left a few more.

jasontedor · 2017-10-11T17:39:39Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+        this.incompatibleClusterStateDiffVersionCount = incompatibleClusterStateDiffVersionCount;
+    }
+
+    public PublishClusterStateStats(StreamInput streamInput) throws IOException {


We normally name these in, it leaves for shorter code and it would be consistent with other places in the codebase.

jasontedor · 2017-10-11T17:40:20Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+        out.writeVLong(incompatibleClusterStateDiffVersionCount);
+    }
+
+    static final class Fields {


We've been moving away from these inner fields classes, we don't need them to encapsulate some strings.

DaveCTurner · 2017-10-11T22:25:32Z

Thanks. I think I've addressed all the comments except for the ones asking for more tests, for which I need a bit more guidance. Not sure I like the inconsistency between incompatibleClusterStateDiffsSentCount and compatibleClusterStateDiffReceivedCount but they were the cleanest things to measure given the existing code. I could do something more complicated to smooth that out if desired.

There is more work to be done in the 6.x branch before the version guard can be reduced like this. This reverts commit f004119.

This reverts commit a9a2828.

DaveCTurner · 2017-10-13T10:34:09Z

@jasontedor @bleskes Could you have another look at this?

bleskes

Thanks @DaveCTurner . This looks great. I left some very minor feedback

bleskes · 2017-10-13T16:55:37Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

@@ -90,6 +98,10 @@
    private final IncomingClusterStateListener incomingClusterStateListener;
    private final DiscoverySettings discoverySettings;

+    private AtomicLong fullClusterStateReceivedCount = new AtomicLong();


can you please make these final?

bleskes · 2017-10-13T16:56:55Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateAction.java

@@ -90,6 +98,10 @@
    private final IncomingClusterStateListener incomingClusterStateListener;
    private final DiscoverySettings discoverySettings;

+    private AtomicLong fullClusterStateReceivedCount = new AtomicLong();
+    private AtomicLong clusterStateDiffReceivedCount = new AtomicLong();
+    private AtomicLong compatibleClusterStateDiffReceivedCount = new AtomicLong();


I personally find having a incompatibleDiff count more handy as it's what we'll be looking for. Maybe just have compatibleDiffCount and incompatibleDiffCount (instead of total)?

Ok. How picky do we want to be about concurrency here? I can do this by counting the cluster state diff as incompatible until successfully applied (i.e. no exceptions thrown) and then to decrement the incompatible count and increment the compatible one, but that does mean you could see incorrect values if you get the stats at the wrong moment.

On second thought the stats() method is already a bit racy.

I have various other, more complicated, ideas:

counting more things, to distinguish in-progress applications from rejected ones.

catching an IncompatibleClusterStateVersionException on the way past and updating the stats in the handler.

keeping the accumulators as-is and calculating the difference in the stats() method (potentially also racy, not sure yet).

introducing locks around stats reads and updates.

Any particular preferences?

bleskes · 2017-10-13T16:59:19Z

rest-api-spec/src/main/resources/rest-api-spec/test/nodes.stats/30_discovery.yml

@@ -26,4 +28,6 @@
  - is_false: nodes.$master.name
  - is_false: nodes.$master.jvm
  - is_true:  nodes.$master.discovery
+  - is_true:  nodes.$master.discovery.cluster_state_queue
+  - is_true:  nodes.$master.discovery.published_cluster_states_received


can we also check the sub elements?

So is_true fails on the subfields because 0 is falsey. I'm leaning towards saying gte: 0 as a proxy for "is present and numeric".

DaveCTurner · 2017-10-17T09:08:40Z

@bleskes changed in b589f36 as requested. I think that's everything.

bleskes

LGTM. If Ci is happy I’m happy.

bleskes · 2017-10-17T09:44:59Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject("published_cluster_states_received");


Nit - strictly speaking these are publishing stats, can we open the object with just published cluster stats (drop received). You can maybe received back in the keys, which can be shorted by dropping the cluster states from the key names - it’s implied from the object they’re in.

DaveCTurner · 2017-10-17T11:46:01Z

@bleskes squashed remaining nits. @jasontedor would you have a look too?

DaveCTurner · 2017-10-24T06:40:22Z

@jasontedor is this ok with you?

jasontedor

I left a few more comments.

jasontedor · 2017-10-24T09:49:38Z

core/src/main/java/org/elasticsearch/discovery/DiscoveryStats.java

@@ -64,4 +80,5 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
    public PendingClusterStateStats getQueueStats() {
        return queueStats;
    }
+    public PublishClusterStateStats getPublishStats() { return publishStats; }


Would you add a newline before this method, and wrap the method definition over multiple lines please?

jasontedor · 2017-10-24T09:51:55Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+        return builder;
+    }
+
+    public long getFullClusterStateReceivedCount() { return fullClusterStateReceivedCount; }


This method can be package-private.

jasontedor · 2017-10-24T09:51:59Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+
+    public long getFullClusterStateReceivedCount() { return fullClusterStateReceivedCount; }
+
+    public long getIncompatibleClusterStateDiffReceivedCount() { return incompatibleClusterStateDiffReceivedCount; }


This method can be package-private.

jasontedor · 2017-10-24T09:52:02Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+
+    public long getIncompatibleClusterStateDiffReceivedCount() { return incompatibleClusterStateDiffReceivedCount; }
+
+    public long getCompatibleClusterStateDiffReceivedCount() { return compatibleClusterStateDiffReceivedCount; }


This method can be package-private.

jasontedor · 2017-10-24T09:52:48Z

core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java

+        builder.field("incompatible_diffs", incompatibleClusterStateDiffReceivedCount);
+        builder.field("compatible_diffs", compatibleClusterStateDiffReceivedCount);
+        builder.endObject();
+        return builder;


Here you can do something like this:

diff --git a/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java b/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java index 356b9a29dc..36794e880f 100644 --- a/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java +++ b/core/src/main/java/org/elasticsearch/discovery/zen/PublishClusterStateStats.java @@ -65,9 +65,11 @@ public class PublishClusterStateStats implements Writeable, ToXContentObject { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject("published_cluster_states"); - builder.field("full_states", fullClusterStateReceivedCount); - builder.field("incompatible_diffs", incompatibleClusterStateDiffReceivedCount); - builder.field("compatible_diffs", compatibleClusterStateDiffReceivedCount); + { + builder.field("full_states", fullClusterStateReceivedCount); + builder.field("incompatible_diffs", incompatibleClusterStateDiffReceivedCount); + builder.field("compatible_diffs", compatibleClusterStateDiffReceivedCount); + } builder.endObject(); return builder; }

which makes the JSON-structure clearer in the code.

jasontedor · 2017-10-24T09:53:53Z

core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java

@@ -413,7 +413,8 @@ public void onNewClusterStateFailed(Exception e) {
    @Override
    public DiscoveryStats stats() {
        PendingClusterStateStats queueStats = pendingStatesQueue.stats();
-        return new DiscoveryStats(queueStats);
+        PublishClusterStateStats publishStats = publishClusterState.stats();
+        return new DiscoveryStats(queueStats, publishStats);


I think:

diff --git a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java index 5f62ee6b97..d688a5d5cd 100644 --- a/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java +++ b/core/src/main/java/org/elasticsearch/discovery/zen/ZenDiscovery.java @@ -412,9 +412,7 @@ public class ZenDiscovery extends AbstractLifecycleComponent implements Discover @Override public DiscoveryStats stats() { - PendingClusterStateStats queueStats = pendingStatesQueue.stats(); - PublishClusterStateStats publishStats = publishClusterState.stats(); - return new DiscoveryStats(queueStats, publishStats); + return new DiscoveryStats(pendingStatesQueue.stats(), publishClusterState.stats()); } public DiscoverySettings getDiscoverySettings() {

is fine?

DaveCTurner · 2017-10-24T12:07:17Z

All addressed @jasontedor - could you review again?

jasontedor

LGTM.

DaveCTurner · 2017-10-25T07:35:52Z

Thanks both. The backport is on its way.

…cessfully (elastic#26973) It's believed that using diffs obsoletes the other mechanism for reusing the bits of the ClusterState that didn't change between updates, but in fact we don't know for sure how often the diff mechanism works successfully. The stats collected here will tell us.

…cessfully (#27107) It's believed that using diffs obsoletes the other mechanism for reusing the bits of the ClusterState that didn't change between updates, but in fact we don't know for sure how often the diff mechanism works successfully. The stats collected here will tell us. Backport of #26973 to 6.x

* master: Ignore .DS_Store files on macOS Docs: Fix ingest geoip config location (elastic#27110) Adjust SHA-512 supported format on plugin install Make ShardSearchTarget optional when parsing ShardSearchFailure (elastic#27078) [Docs] Clarify mapping `index` option default (elastic#27104) Decouple BulkProcessor from ThreadPool (elastic#26727) Stats to record how often the ClusterState diff mechanism is used successfully (elastic#26973) Tie-break shard path decision based on total number of shards on path (elastic#27039)

DaveCTurner requested a review from bleskes October 11, 2017 16:51

DaveCTurner added :Data Management/Stats Statistics tracking and retrieval APIs v7.0.0 labels Oct 11, 2017

Picky picky

5ed348a

bleskes suggested changes Oct 11, 2017

View reviewed changes

jasontedor reviewed Oct 11, 2017

View reviewed changes

DaveCTurner added 6 commits October 11, 2017 18:55

No need for Fields class

db71be7

Shorten arg name

092be64

XContent not XContentFragment

8675140

Make accumulators into AtomicLongs

88bfda5

Add javadoc

d825792

Slated for inclusion in 6.1

f004119

DaveCTurner added v6.1.0 backport pending labels Oct 11, 2017

Add stats for receivers of cluster states too

ec982bd

DaveCTurner added 4 commits October 12, 2017 00:29

Nit: Diffs -> Diff

c7a0192

Misread comment - should have been ToXContentObject

660f20a

Don't bother collecting the master-side stats at all

5543e88

Stick with v7 for now.

67c1a22

There is more work to be done in the 6.x branch before the version guard can be reduced like this. This reverts commit f004119.

DaveCTurner removed the backport pending label Oct 12, 2017

DaveCTurner added 6 commits October 12, 2017 16:38

Missed one

e0f3da3

Add assertions that the stats are updated as expected

a9a2828

Remove assertions from existing test

1852f8f

This reverts commit a9a2828.

Add testPublishClusterStateStats()

34c3b5a

Better grammar

6af7728

Add REST assertions

b935508

bleskes suggested changes Oct 13, 2017

View reviewed changes

DaveCTurner added 4 commits October 16, 2017 12:00

No need for @nullables

a5124e0

Skip these tests until v7.x

d61551f

Line length oops

3dd4219

Count incompatible states received in exception handler instead

b589f36

bleskes approved these changes Oct 17, 2017

View reviewed changes

DaveCTurner added 2 commits October 17, 2017 13:41

Shorter keys in PublishClusterStateStats#toXContent()

ed570df

Fewer linebreaks

322c1de

Fixed up REST API test

840a5bd

jasontedor reviewed Oct 24, 2017

View reviewed changes

DaveCTurner added 4 commits October 24, 2017 11:09

Merge branch 'master' into 2017-10-11-discovery-stats

54b840a

Formatting

1fa86b7

Inline

ff54a0d

Make getters package-private

168d94a

jasontedor approved these changes Oct 24, 2017

View reviewed changes

DaveCTurner merged commit cc3364e into elastic:master Oct 25, 2017

DaveCTurner mentioned this pull request Oct 25, 2017

Stats to record how often the ClusterState diff mechanism is used successfully #27107

Merged

clintongormley added >enhancement and removed v6.1.0 labels Dec 12, 2017

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

DaveCTurner deleted the 2017-10-11-discovery-stats branch July 23, 2022 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stats to record how often the ClusterState diff mechanism is used successfully #26973

Stats to record how often the ClusterState diff mechanism is used successfully #26973

DaveCTurner commented Oct 11, 2017 •

edited

Loading

bleskes left a comment

bleskes Oct 11, 2017

jasontedor Oct 11, 2017

bleskes Oct 11, 2017

bleskes Oct 11, 2017

DaveCTurner Oct 11, 2017

jasontedor left a comment

jasontedor Oct 11, 2017

jasontedor Oct 11, 2017

DaveCTurner commented Oct 11, 2017

DaveCTurner commented Oct 13, 2017

bleskes left a comment

bleskes Oct 13, 2017

bleskes Oct 13, 2017

DaveCTurner Oct 16, 2017

bleskes Oct 13, 2017

DaveCTurner Oct 16, 2017

DaveCTurner commented Oct 17, 2017

bleskes left a comment

bleskes Oct 17, 2017

DaveCTurner commented Oct 17, 2017

DaveCTurner commented Oct 24, 2017

jasontedor left a comment

jasontedor Oct 24, 2017

jasontedor Oct 24, 2017

jasontedor Oct 24, 2017

jasontedor Oct 24, 2017

jasontedor Oct 24, 2017

jasontedor Oct 24, 2017

DaveCTurner commented Oct 24, 2017

jasontedor left a comment

DaveCTurner commented Oct 25, 2017


		public long getFullClusterStateReceivedCount() { return fullClusterStateReceivedCount; }

		public long getIncompatibleClusterStateDiffReceivedCount() { return incompatibleClusterStateDiffReceivedCount; }


		public long getIncompatibleClusterStateDiffReceivedCount() { return incompatibleClusterStateDiffReceivedCount; }

		public long getCompatibleClusterStateDiffReceivedCount() { return compatibleClusterStateDiffReceivedCount; }

Stats to record how often the ClusterState diff mechanism is used successfully #26973

Stats to record how often the ClusterState diff mechanism is used successfully #26973

Conversation

DaveCTurner commented Oct 11, 2017 • edited Loading

bleskes 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

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 11, 2017

DaveCTurner commented Oct 13, 2017

bleskes 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

DaveCTurner commented Oct 17, 2017

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 17, 2017

DaveCTurner commented Oct 24, 2017

jasontedor 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

DaveCTurner commented Oct 24, 2017

jasontedor left a comment

Choose a reason for hiding this comment

DaveCTurner commented Oct 25, 2017

DaveCTurner commented Oct 11, 2017 •

edited

Loading