Report transport message size per action #94543

DaveCTurner · 2023-03-20T11:12:17Z

Adds to the transport stats a histogram of transport message sizes for each transport action.

Closes #88151

Adds to the transport stats a histogram of transport message sizes for each transport action.

elasticsearchmachine · 2023-03-20T11:12:42Z

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

elasticsearchmachine · 2023-03-20T11:12:43Z

Pinging @elastic/es-distributed (Team:Distributed)

henningandersen

Left a few minor comments. I wonder if we should add a brief .yml test to validate the structure of the response. And also, I think we can add documentation for the new stats in this PR.

server/src/main/java/org/elasticsearch/transport/InboundHandler.java

henningandersen · 2023-03-20T12:36:11Z

server/src/main/java/org/elasticsearch/transport/InboundHandler.java

+                    requestId,
+                    version,
+                    header.getCompressionScheme(),
+                    ResponseStatsConsumer.NONE,


Can we add the response stats instead here, except if short circuited?

I suspect we should track the response stats if short-circuited too? 905e5ad does that, but I can rework this some more if you'd prefer to drop the response stats for short-circuited ones.

Ok I see, we short-circuit if the action is not known, as well as on other up-front exceptions. afaa718 captures the response stats for known actions even if short-circuited.

henningandersen · 2023-03-20T12:46:38Z

server/src/main/java/org/elasticsearch/transport/TransportActionStatsTracker.java

+     *   n:  < 2^(n+3)
+     * ...
+     *  25:  < 2^28 == 256MiB
+     *  26: >= 2^28


Are we missing 2 buckets? >= 512MiB and >= 1GiB?

I figured by the time we got to 256MiB the messages were already unreasonably large, but you're right it's only 2 buckets away from the max size anyway. Added in 80eeac4.

henningandersen · 2023-03-20T12:53:45Z

server/src/main/java/org/elasticsearch/transport/TransportStats.java

@@ -41,7 +43,8 @@ public TransportStats(
        long txCount,
        long txSize,
        long[] inboundHandlingTimeBucketFrequencies,
-        long[] outboundHandlingTimeBucketFrequencies
+        long[] outboundHandlingTimeBucketFrequencies,
+        List<TransportActionStats> transportActionStats


I think it would be more intuitive to have the stats be a Map - also to avoid that TransportActionStats is a fragment.

Sure, done in f2e42f1

henningandersen · 2023-03-20T12:56:31Z

server/src/main/java/org/elasticsearch/transport/TransportStats.java

+            // Stats came from before v8.7
+            assert Version.CURRENT.major == Version.V_8_0_0.major;


I am not exactly sure what the purpose here is? I am guessing it is to ensure that we cleanup something here when major switches to 9?

Yeah except it should have mentioned 7.0 not 8.0. Fixed now.

henningandersen · 2023-03-20T14:52:10Z

server/src/main/java/org/elasticsearch/transport/TransportActionStats.java

+        return builder.endObject();
+    }
+
+    static void histogramToXContent(XContentBuilder builder, long[] sizeHistogram, String fieldName) throws IOException {


The fieldName seems to be passed "histogram" always except in tests, could it not be hardcoded instead?

Sure, I think I used the shorter one to avoid some line breaking mess but it's not necessary any more. Done in e24b97e.

idegtiarenko · 2023-03-20T15:17:34Z

server/src/main/java/org/elasticsearch/transport/TransportActionStats.java

+        final int[] bucketBounds = TransportActionStatsTracker.getBucketUpperBounds();
+        assert sizeHistogram.length == bucketBounds.length + 1;
+        builder.startArray(fieldName);
+
+        int firstBucket = 0;
+        long remainingCount = 0L;
+        for (int i = 0; i < sizeHistogram.length; i++) {
+            if (remainingCount == 0) {
+                firstBucket = i;
+            }
+            remainingCount += sizeHistogram[i];
+        }
+
+        for (int i = firstBucket; i < sizeHistogram.length && 0 < remainingCount; i++) {
+            builder.startObject();
+            if (i > 0) {
+                builder.humanReadableField("ge_bytes", "ge", ByteSizeValue.ofBytes(bucketBounds[i - 1]));
+            }
+            if (i < bucketBounds.length) {
+                builder.humanReadableField("lt_bytes", "lt", ByteSizeValue.ofBytes(bucketBounds[i]));
+            }
+            builder.field("count", sizeHistogram[i]);
+            builder.endObject();
+            remainingCount -= sizeHistogram[i];
+        }
+        builder.endArray();


Should we consider org.HdrHistogram.Histogram to simplify rendering json and omitting empty buckets and also for determining the bucket by the size?

I'm not sure I understand how this could make things simpler, but maybe I'm missing something.

idegtiarenko · 2023-03-23T07:09:14Z

server/src/main/java/org/elasticsearch/transport/Transport.java

-                .sorted(Comparator.comparing(RequestHandlerRegistry::getAction))
-                .map(RequestHandlerRegistry::getStats)
-                .toList();
+                .collect(Collectors.toMap(RequestHandlerRegistry::getAction, RequestHandlerRegistry::getStats, (a, b) -> a, TreeMap::new));


Re (a, b) -> a, do we expect a duplicate action somewhere?

no, but we've got to say something here (there's no overload that accepts a mapFactory without a mergeFunction) and this is the shortest thing we can say

Oh, okay. NIT: we have org.elasticsearch.common.util.Maps#toUnmodifiableSortedMap that only accept key/value functions and could be used here.

TIL thanks, that's what I wanted 😄

elasticsearchmachine · 2023-03-23T15:30:38Z

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

henningandersen

LGTM.

henningandersen · 2023-03-28T11:18:01Z

docs/reference/cluster/nodes-stats.asciidoc

+showing the total amount of traffic and a histogram of message sizes for
+incoming requests and outgoing responses.
+
+.Properties of `actions.*.requests` and `actions.*.responses`


The below seems to describe the histograms, can we make that explicit here? And also include documentation of the outer level count and total_size?

Oh oops good catch. Fixed in 63d5d73.

henningandersen · 2023-03-28T11:23:39Z

server/src/main/java/org/elasticsearch/transport/InboundHandler.java

+                    header.isHandshake(),
+                    message.takeBreakerReleaseControl()
+                );
+            } catch (Exception e) {


I am not sure I see a potential for exceptions in the code above. Fine to keep this, but if there is no exception expected, perhaps we can assert false here?

Yes I couldn't either but I'm paranoid about leaking a response here (or in future). Assertion added in 20ee73d.

henningandersen · 2023-03-28T11:32:05Z

server/src/main/java/org/elasticsearch/transport/TransportActionStats.java

+import java.io.IOException;
+
+public record TransportActionStats(
+    String action,


This field is no longer used and I think we should remove it.

Good point, done in c1247b0.

henningandersen · 2023-03-28T11:41:00Z

server/src/main/java/org/elasticsearch/transport/TransportStats.java

-            assert Version.CURRENT.major == Version.V_8_0_0.major;
+            assert Version.CURRENT.major == Version.V_7_0_0.major + 1;


Not sure I understand the difference, can you explain the motivation for changing this?

When Version.CURRENT becomes 9.0 we remove all the v7.x version constants, so by mentioning one here we are forced to remove this dead code to fix the compile error.

Report transport message size per action

5be7200

Adds to the transport stats a histogram of transport message sizes for each transport action.

DaveCTurner added >enhancement :Distributed Coordination/Network Http and internode communication implementations v8.8.0 labels Mar 20, 2023

DaveCTurner requested a review from henningandersen March 20, 2023 11:12

elasticsearchmachine added the Team:Distributed Meta label for distributed team (obsolete) label Mar 20, 2023

Update docs/changelog/94543.yaml

02daec1

henningandersen reviewed Mar 20, 2023

View reviewed changes

idegtiarenko reviewed Mar 20, 2023

View reviewed changes

DaveCTurner added 7 commits March 22, 2023 13:13

Merge branch 'main' into 2023-03-19-per-action-transport-stats

e9bc959

Buckets go all the way to 1GiB

80eeac4

Fail compile in next major

d9c73e1

Always call it a histogram

e24b97e

Move to map

f2e42f1

Track stats for all kinds of error responses too

905e5ad

Not a fragment

613f657

DaveCTurner requested a review from henningandersen March 22, 2023 14:07

DaveCTurner added 2 commits March 22, 2023 15:08

Test fixes

5aac032

Unknown actions are short-circuited

afaa718

idegtiarenko reviewed Mar 23, 2023

View reviewed changes

DaveCTurner added 6 commits March 23, 2023 08:41

Merge branch 'main' into 2023-03-19-per-action-transport-stats

42c6869

Maps.toUnmodifiableSortedMap

7d3dac6

YAML test

39f3083

Human YAML test

78f8faf

Docs

34be32d

Update docs/changelog/94543.yaml

4d6809b

henningandersen approved these changes Mar 28, 2023

View reviewed changes

DaveCTurner added 4 commits March 28, 2023 15:59

Merge branch 'main' into 2023-03-19-per-action-transport-stats

fd6e099

Assert false in impossible catch

20ee73d

Docs

63d5d73

(effectively) unused

c1247b0

DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 28, 2023

elasticsearchmachine merged commit 4ef9965 into elastic:main Mar 28, 2023

DaveCTurner deleted the 2023-03-19-per-action-transport-stats branch March 28, 2023 16:05

DaveCTurner mentioned this pull request Mar 28, 2023

Trace transport request/response sizes in APM #94845

Open

b-deam mentioned this pull request Jun 1, 2023

Increase 'mapping.total_fields.limit' to '2000' elastic/rally#1722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report transport message size per action #94543

Report transport message size per action #94543

DaveCTurner commented Mar 20, 2023 •

edited

Loading

elasticsearchmachine commented Mar 20, 2023

elasticsearchmachine commented Mar 20, 2023

henningandersen left a comment

henningandersen Mar 20, 2023

DaveCTurner Mar 22, 2023

DaveCTurner Mar 23, 2023

henningandersen Mar 20, 2023

DaveCTurner Mar 22, 2023

henningandersen Mar 20, 2023

DaveCTurner Mar 22, 2023

henningandersen Mar 20, 2023

DaveCTurner Mar 22, 2023

henningandersen Mar 20, 2023

DaveCTurner Mar 22, 2023

idegtiarenko Mar 20, 2023 •

edited

Loading

DaveCTurner Mar 22, 2023

idegtiarenko Mar 23, 2023

DaveCTurner Mar 23, 2023

idegtiarenko Mar 23, 2023

DaveCTurner Mar 23, 2023

elasticsearchmachine commented Mar 23, 2023

henningandersen left a comment

henningandersen Mar 28, 2023

DaveCTurner Mar 28, 2023

henningandersen Mar 28, 2023

DaveCTurner Mar 28, 2023

henningandersen Mar 28, 2023

DaveCTurner Mar 28, 2023

henningandersen Mar 28, 2023

DaveCTurner Mar 28, 2023

		// Stats came from before v8.7
		assert Version.CURRENT.major == Version.V_8_0_0.major;

Report transport message size per action #94543

Report transport message size per action #94543

Conversation

DaveCTurner commented Mar 20, 2023 • edited Loading

elasticsearchmachine commented Mar 20, 2023

elasticsearchmachine commented Mar 20, 2023

henningandersen 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

idegtiarenko Mar 20, 2023 • edited Loading

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 Mar 23, 2023

henningandersen 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

DaveCTurner commented Mar 20, 2023 •

edited

Loading

idegtiarenko Mar 20, 2023 •

edited

Loading