Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify TransportStats assertions in v9 #114700

Merged
merged 19 commits into from
Feb 5, 2025
Merged

Simplify TransportStats assertions in v9 #114700

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
00f255d
Simplfy TransportStats assertions in v9
arteam Oct 14, 2024
d92ee2d
Merge remote-tracking branch 'origin/main' into v9-transport-stats-as…
arteam Oct 15, 2024
7c78d05
Merge branch 'main' into v9-transport-stats-assertions
elasticmachine Oct 16, 2024
55fa922
Keep condition checks for transport stats assertions in v9
arteam Oct 16, 2024
49dbf89
Merge branch 'main' into v9-transport-stats-assertions
elasticmachine Oct 16, 2024
ffc518b
Merge remote-tracking branch 'origin/main' into v9-transport-stats-as…
arteam Oct 23, 2024
578ccae
Simplify assertions
arteam Oct 23, 2024
44fd7bd
Merge remote-tracking branch 'origin/v9-transport-stats-assertions' i…
arteam Oct 23, 2024
45f47f3
Test transport stats in 9.0 format
arteam Oct 23, 2024
237f6e7
Simplify consistency checks
arteam Oct 24, 2024
a2ac637
Merge remote-tracking branch 'origin/main' into v9-transport-stats-as…
arteam Oct 24, 2024
848b627
Merge remote-tracking branch 'origin/main' into v9-transport-stats-as…
arteam Nov 4, 2024
35a2901
Assert transportActionStats is not empty
arteam Nov 4, 2024
d15016e
Revert "Assert transportActionStats is not empty"
arteam Nov 4, 2024
2d28949
Merge branch 'main' into v9-transport-stats-assertions
arteam Jan 24, 2025
9dbe154
Merge branch 'main' into v9-transport-stats-assertions
arteam Feb 3, 2025
bb59b3b
Remove version 8_1 checks
arteam Feb 3, 2025
de26a70
Merge branch 'main' into v9-transport-stats-assertions
arteam Feb 5, 2025
efc71fd
Ensure that inbound and outbound bucket should have the length of BUC…
arteam Feb 5, 2025
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
2 changes: 2 additions & 0 deletions server/src/main/java/org/elasticsearch/TransportVersions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ static TransportVersion def(int id) {
public static final TransportVersion COHERE_BIT_EMBEDDING_TYPE_SUPPORT_ADDED_BACKPORT_8_X = def(8_840_0_01);
public static final TransportVersion ELASTICSEARCH_9_0 = def(9_000_0_00);
public static final TransportVersion COHERE_BIT_EMBEDDING_TYPE_SUPPORT_ADDED = def(9_001_0_00);
public static final TransportVersion TRANSPORT_STATS_HANDLING_TIME_REQUIRED = def(9_002_0_00);

/*
* STOP! READ THIS FIRST! No, really,
* ____ _____ ___ ____ _ ____ _____ _ ____ _____ _ _ ___ ____ _____ ___ ____ ____ _____ _
Expand Down
76 changes: 24 additions & 52 deletions server/src/main/java/org/elasticsearch/transport/TransportStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import org.elasticsearch.common.unit.ByteSizeValue;
import org.elasticsearch.common.xcontent.ChunkedToXContent;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;

Expand Down Expand Up @@ -70,18 +69,16 @@ public TransportStats(StreamInput in) throws IOException {
rxSize = in.readVLong();
txCount = in.readVLong();
txSize = in.readVLong();
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_1_0) && in.readBoolean()) {
inboundHandlingTimeBucketFrequencies = new long[HandlingTimeTracker.BUCKET_COUNT];
for (int i = 0; i < inboundHandlingTimeBucketFrequencies.length; i++) {
inboundHandlingTimeBucketFrequencies[i] = in.readVLong();
}
outboundHandlingTimeBucketFrequencies = new long[HandlingTimeTracker.BUCKET_COUNT];
for (int i = 0; i < inboundHandlingTimeBucketFrequencies.length; i++) {
outboundHandlingTimeBucketFrequencies[i] = in.readVLong();
}
} else {
inboundHandlingTimeBucketFrequencies = new long[0];
outboundHandlingTimeBucketFrequencies = new long[0];
if (in.getTransportVersion().before(TransportVersions.TRANSPORT_STATS_HANDLING_TIME_REQUIRED)) {
in.readBoolean();
}
inboundHandlingTimeBucketFrequencies = new long[HandlingTimeTracker.BUCKET_COUNT];
for (int i = 0; i < inboundHandlingTimeBucketFrequencies.length; i++) {
inboundHandlingTimeBucketFrequencies[i] = in.readVLong();
}
outboundHandlingTimeBucketFrequencies = new long[HandlingTimeTracker.BUCKET_COUNT];
for (int i = 0; i < inboundHandlingTimeBucketFrequencies.length; i++) {
outboundHandlingTimeBucketFrequencies[i] = in.readVLong();
}
if (in.getTransportVersion().onOrAfter(TransportVersions.V_8_8_0)) {
transportActionStats = Collections.unmodifiableMap(in.readOrderedMap(StreamInput::readString, TransportActionStats::new));
Expand All @@ -99,15 +96,15 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeVLong(rxSize);
out.writeVLong(txCount);
out.writeVLong(txSize);
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_1_0)) {
assert (inboundHandlingTimeBucketFrequencies.length > 0) == (outboundHandlingTimeBucketFrequencies.length > 0);
assert (inboundHandlingTimeBucketFrequencies.length > 0) == (outboundHandlingTimeBucketFrequencies.length > 0);
if (out.getTransportVersion().before(TransportVersions.TRANSPORT_STATS_HANDLING_TIME_REQUIRED)) {
out.writeBoolean(inboundHandlingTimeBucketFrequencies.length > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't match up with the reading code which requires these things to have length HandlingTimeTracker.BUCKET_COUNT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've hardened up the assertions on the bucket frequencies length in efc71fd

for (long handlingTimeBucketFrequency : inboundHandlingTimeBucketFrequencies) {
out.writeVLong(handlingTimeBucketFrequency);
}
for (long handlingTimeBucketFrequency : outboundHandlingTimeBucketFrequencies) {
out.writeVLong(handlingTimeBucketFrequency);
}
}
for (long handlingTimeBucketFrequency : inboundHandlingTimeBucketFrequencies) {
out.writeVLong(handlingTimeBucketFrequency);
}
for (long handlingTimeBucketFrequency : outboundHandlingTimeBucketFrequencies) {
out.writeVLong(handlingTimeBucketFrequency);
}
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_8_0)) {
out.writeMap(transportActionStats, StreamOutput::writeWriteable);
Expand Down Expand Up @@ -166,24 +163,13 @@ public Map<String, TransportActionStats> getTransportActionStats() {
return transportActionStats;
}

@UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION)
// Review and simplify the if-else blocks containing this symbol once v9 is released
private static final boolean IMPOSSIBLE_IN_V9 = true;

private boolean assertHistogramsConsistent() {
assert inboundHandlingTimeBucketFrequencies.length == outboundHandlingTimeBucketFrequencies.length;
if (inboundHandlingTimeBucketFrequencies.length == 0) {
// Stats came from before v8.1
assert IMPOSSIBLE_IN_V9;
} else {
assert inboundHandlingTimeBucketFrequencies.length == HandlingTimeTracker.BUCKET_COUNT;
}
assert inboundHandlingTimeBucketFrequencies.length == HandlingTimeTracker.BUCKET_COUNT;
return true;
}

@Override
@UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_COORDINATION)
// review the "if" blocks checking for non-empty once we have
public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerParams) {
return Iterators.concat(Iterators.single((builder, params) -> {
builder.startObject(Fields.TRANSPORT);
Expand All @@ -193,19 +179,10 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP
builder.humanReadableField(Fields.RX_SIZE_IN_BYTES, Fields.RX_SIZE, ByteSizeValue.ofBytes(rxSize));
builder.field(Fields.TX_COUNT, txCount);
builder.humanReadableField(Fields.TX_SIZE_IN_BYTES, Fields.TX_SIZE, ByteSizeValue.ofBytes(txSize));
if (inboundHandlingTimeBucketFrequencies.length > 0) {
histogramToXContent(builder, inboundHandlingTimeBucketFrequencies, Fields.INBOUND_HANDLING_TIME_HISTOGRAM);
histogramToXContent(builder, outboundHandlingTimeBucketFrequencies, Fields.OUTBOUND_HANDLING_TIME_HISTOGRAM);
} else {
// Stats came from before v8.1
assert IMPOSSIBLE_IN_V9;
}
if (transportActionStats.isEmpty() == false) {
builder.startObject(Fields.ACTIONS);
} else {
// Stats came from before v8.8
assert IMPOSSIBLE_IN_V9;
}
assert inboundHandlingTimeBucketFrequencies.length > 0;
histogramToXContent(builder, inboundHandlingTimeBucketFrequencies, Fields.INBOUND_HANDLING_TIME_HISTOGRAM);
histogramToXContent(builder, outboundHandlingTimeBucketFrequencies, Fields.OUTBOUND_HANDLING_TIME_HISTOGRAM);
builder.startObject(Fields.ACTIONS);
return builder;
}),

Expand All @@ -215,12 +192,7 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params outerP
return builder;
}),

Iterators.single((builder, params) -> {
if (transportActionStats.isEmpty() == false) {
builder.endObject();
}
return builder.endObject();
})
Iterators.single((builder, params) -> { return builder.endObject().endObject(); })
);
}

Expand Down
48 changes: 4 additions & 44 deletions server/src/test/java/org/elasticsearch/transport/TransportStatsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,50 +20,8 @@

public class TransportStatsTests extends ESTestCase {
public void testToXContent() {
assertEquals(
Strings.toString(
new TransportStats(1, 2, 3, ByteSizeUnit.MB.toBytes(4), 5, ByteSizeUnit.MB.toBytes(6), new long[0], new long[0], Map.of()),
false,
true
),
"""
{"transport":{"server_open":1,"total_outbound_connections":2,\
"rx_count":3,"rx_size":"4mb","rx_size_in_bytes":4194304,\
"tx_count":5,"tx_size":"6mb","tx_size_in_bytes":6291456\
}}"""
);

final var histogram = new long[HandlingTimeTracker.BUCKET_COUNT];
assertEquals(
Strings.toString(
new TransportStats(1, 2, 3, ByteSizeUnit.MB.toBytes(4), 5, ByteSizeUnit.MB.toBytes(6), histogram, histogram, Map.of()),
false,
true
),
"""
{"transport":{"server_open":1,"total_outbound_connections":2,\
"rx_count":3,"rx_size":"4mb","rx_size_in_bytes":4194304,\
"tx_count":5,"tx_size":"6mb","tx_size_in_bytes":6291456,\
"inbound_handling_time_histogram":[],\
"outbound_handling_time_histogram":[]\
}}"""
);

histogram[4] = 10;
assertEquals(
Strings.toString(
new TransportStats(1, 2, 3, ByteSizeUnit.MB.toBytes(4), 5, ByteSizeUnit.MB.toBytes(6), histogram, histogram, Map.of()),
false,
true
),
"""
{"transport":{"server_open":1,"total_outbound_connections":2,\
"rx_count":3,"rx_size":"4mb","rx_size_in_bytes":4194304,\
"tx_count":5,"tx_size":"6mb","tx_size_in_bytes":6291456,\
"inbound_handling_time_histogram":[{"ge":"8ms","ge_millis":8,"lt":"16ms","lt_millis":16,"count":10}],\
"outbound_handling_time_histogram":[{"ge":"8ms","ge_millis":8,"lt":"16ms","lt_millis":16,"count":10}]\
}}"""
);

final var requestSizeHistogram = new long[29];
requestSizeHistogram[2] = 9;
Expand All @@ -84,8 +42,8 @@ public void testToXContent() {
ByteSizeUnit.MB.toBytes(4),
5,
ByteSizeUnit.MB.toBytes(6),
new long[0],
new long[0],
histogram,
histogram,
Map.of("internal:test/action", exampleActionStats)
),
false,
Expand All @@ -95,6 +53,8 @@ public void testToXContent() {
{"transport":{"server_open":1,"total_outbound_connections":2,\
"rx_count":3,"rx_size":"4mb","rx_size_in_bytes":4194304,\
"tx_count":5,"tx_size":"6mb","tx_size_in_bytes":6291456,\
"inbound_handling_time_histogram":[{"ge":"8ms","ge_millis":8,"lt":"16ms","lt_millis":16,"count":10}],\
"outbound_handling_time_histogram":[{"ge":"8ms","ge_millis":8,"lt":"16ms","lt_millis":16,"count":10}],\
"actions":{"internal:test/action":%s}}}""", Strings.toString(exampleActionStats, false, true))
);
}
Expand Down