Skip to content

Commit 51a0709

Browse files
szybiaszybia
authored
ILM Explain: valid JSON on truncated step info (elastic#137638) (elastic#138606)
1 parent 394ea7d commit 51a0709

File tree

6 files changed

+172
-29
lines changed

6 files changed

+172
-29
lines changed

‎docs/changelog/137638.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 137638
2+
summary: "ILM Explain: valid JSON on truncated step info"
3+
area: ILM+SLM
4+
type: bug
5+
issues:
6+
- 135458

‎server/src/main/java/org/elasticsearch/cluster/metadata/LifecycleExecutionState.java‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -269,15 +269,31 @@ public Map<String, String> asMap() {
269269
return Collections.unmodifiableMap(result);
270270
}
271271

272-
public static String truncateWithExplanation(String input) {
273-
if (input != null && input.length() > MAXIMUM_STEP_INFO_STRING_LENGTH) {
274-
return Strings.cleanTruncate(input, MAXIMUM_STEP_INFO_STRING_LENGTH)
275-
+ "... ("
276-
+ (input.length() - MAXIMUM_STEP_INFO_STRING_LENGTH)
277-
+ " chars truncated)";
278-
} else {
279-
return input;
280-
}
272+
/**
273+
* Truncates a potentially long JSON string to ensure it does not exceed {@link #MAXIMUM_STEP_INFO_STRING_LENGTH}. If truncation
274+
* occurs, an explanation suffix is appended to the truncated string indicating <i>approximately</i> how many characters were removed.
275+
* We return an approximation because we're valuing code simplicity over accuracy in this area.
276+
*
277+
* @param json the JSON string to potentially truncate
278+
* @return the original JSON string if its length is within the limit, otherwise a truncated version with an explanation suffix - in
279+
* correct JSON format
280+
*/
281+
public static String potentiallyTruncateLongJsonWithExplanation(String json) {
282+
if (json == null || json.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH) {
283+
return json;
284+
}
285+
// we'll now do our best to truncate the JSON and have the result be valid JSON.
286+
// a long input JSON should generally only be possible with an exception turned into JSON that's well-formatted.
287+
// if we fail to produce valid JSON, we might return invalid JSON in REST API in niche cases, so this isn't catastrophic.
288+
assert json.startsWith("{\"") && json.endsWith("\"}") : "expected more specific JSON format, might produce invalid JSON";
289+
final int roughNumberOfCharsTruncated = json.length() - MAXIMUM_STEP_INFO_STRING_LENGTH;
290+
final String truncationExplanation = "... (~" + roughNumberOfCharsTruncated + " chars truncated)\"}";
291+
// To ensure that the resulting string is always <= the max, we also need to remove the length of our suffix.
292+
// This means that the actual number of characters removed is `truncationExplanation.length()` more than we say it is.
293+
final String truncated = Strings.cleanTruncate(json, MAXIMUM_STEP_INFO_STRING_LENGTH - truncationExplanation.length())
294+
+ truncationExplanation;
295+
assert truncated.length() <= MAXIMUM_STEP_INFO_STRING_LENGTH : "truncation didn't work";
296+
return truncated;
281297
}
282298

283299
public static class Builder {
@@ -321,7 +337,7 @@ public Builder setFailedStep(String failedStep) {
321337
}
322338

323339
public Builder setStepInfo(String stepInfo) {
324-
this.stepInfo = truncateWithExplanation(stepInfo);
340+
this.stepInfo = potentiallyTruncateLongJsonWithExplanation(stepInfo);
325341
return this;
326342
}
327343

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecycleExecutionStateTests.java‎

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,18 @@
88
package org.elasticsearch.xpack.core.ilm;
99

1010
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
11+
import org.elasticsearch.common.Strings;
1112
import org.elasticsearch.test.ESTestCase;
1213
import org.elasticsearch.test.EqualsHashCodeTestUtils;
1314

1415
import java.util.HashMap;
1516
import java.util.Map;
1617

18+
import static org.hamcrest.Matchers.endsWith;
1719
import static org.hamcrest.Matchers.equalTo;
20+
import static org.hamcrest.Matchers.hasLength;
21+
import static org.hamcrest.Matchers.not;
22+
import static org.hamcrest.Matchers.sameInstance;
1823

1924
public class LifecycleExecutionStateTests extends ESTestCase {
2025

@@ -28,14 +33,12 @@ public void testTruncatingStepInfo() {
2833
Map<String, String> custom = createCustomMetadata();
2934
LifecycleExecutionState state = LifecycleExecutionState.fromCustomMetadata(custom);
3035
assertThat(custom.get("step_info"), equalTo(state.stepInfo()));
31-
String longStepInfo = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 100);
36+
String longStepInfo = "{\"key\": \""
37+
+ randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 100)
38+
+ "\"}";
3239
LifecycleExecutionState newState = LifecycleExecutionState.builder(state).setStepInfo(longStepInfo).build();
33-
// Length includes the post suffix
34-
assertThat(newState.stepInfo().length(), equalTo(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH + 25));
35-
assertThat(
36-
newState.stepInfo().substring(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH, 1049),
37-
equalTo("... (100 chars truncated)")
38-
);
40+
assertThat(newState.stepInfo(), hasLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH));
41+
assertThat(newState.stepInfo(), endsWith("... (~111 chars truncated)\"}"));
3942
}
4043

4144
public void testEmptyValuesAreNotSerialized() {
@@ -145,6 +148,59 @@ public void testGetCurrentStepKey() {
145148
assertNull(error6.getMessage());
146149
}
147150

151+
/** test that strings with length less than or equal to maximum string length are not truncated and returned as-is */
152+
public void testPotentiallyTruncateLongJsonWithExplanationNoNeedToTruncate() {
153+
final String input = randomAlphaOfLengthBetween(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
154+
assertSame(input, LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input));
155+
}
156+
157+
/** test that string with length one character over the max limit has truncation applied to it and has correct explanation */
158+
public void testPotentiallyTruncateLongJsonWithExplanationOneCharTruncated() {
159+
final String jsonBaseFormat = "{\"key\": \"%s\"}";
160+
final int baseLength = Strings.format(jsonBaseFormat, "").length();
161+
final String value = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 1);
162+
final String input = Strings.format(jsonBaseFormat, value);
163+
164+
final String expectedSuffix = "... (~1 chars truncated)";
165+
final String expectedOutput = Strings.format(
166+
jsonBaseFormat,
167+
value.substring(0, value.length() - expectedSuffix.length() - 1) + expectedSuffix
168+
);
169+
final String actualOutput = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input);
170+
assertThat(actualOutput, hasLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH));
171+
assertEquals(expectedOutput, actualOutput);
172+
}
173+
174+
/** test that string with length two characters over the max limit has truncation applied to it and has correct explanation */
175+
public void testPotentiallyTruncateLongJsonWithExplanationTwoCharsTruncated() {
176+
final String jsonBaseFormat = "{\"key\": \"%s\"}";
177+
final int baseLength = Strings.format(jsonBaseFormat, "").length();
178+
final String value = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - baseLength + 2);
179+
final String input = Strings.format(jsonBaseFormat, value);
180+
181+
final String expectedSuffix = "... (~2 chars truncated)";
182+
final String expectedOutput = Strings.format(
183+
jsonBaseFormat,
184+
value.substring(0, value.length() - expectedSuffix.length() - 2) + expectedSuffix
185+
);
186+
final String actualOutput = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input);
187+
assertThat(actualOutput, hasLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH));
188+
assertEquals(expectedOutput, actualOutput);
189+
}
190+
191+
public void testPotentiallyTruncateLongJsonWithExplanationIsIdempotent() {
192+
final String input = "{\"key\": \"" + randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH) + "\"}";
193+
194+
final String firstOutput = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(input);
195+
196+
assertThat(firstOutput, hasLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH));
197+
assertThat(firstOutput, not(equalTo(input)));
198+
199+
final String secondOutput = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(firstOutput);
200+
201+
assertThat(secondOutput, sameInstance(firstOutput));
202+
}
203+
148204
private LifecycleExecutionState mutate(LifecycleExecutionState toMutate) {
149205
LifecycleExecutionState.Builder newState = LifecycleExecutionState.builder(toMutate);
150206
switch (randomIntBetween(0, 18)) {

‎x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/ExplainLifecycleIT.java‎

Lines changed: 74 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,10 @@
1010
import org.apache.http.entity.ContentType;
1111
import org.apache.http.entity.StringEntity;
1212
import org.apache.http.util.EntityUtils;
13-
import org.apache.logging.log4j.LogManager;
14-
import org.apache.logging.log4j.Logger;
1513
import org.elasticsearch.client.Request;
1614
import org.elasticsearch.client.Response;
1715
import org.elasticsearch.cluster.metadata.IndexMetadata;
16+
import org.elasticsearch.cluster.metadata.LifecycleExecutionState;
1817
import org.elasticsearch.common.Strings;
1918
import org.elasticsearch.common.settings.Settings;
2019
import org.elasticsearch.core.TimeValue;
@@ -44,6 +43,7 @@
4443
import static org.elasticsearch.xpack.TimeSeriesRestDriver.explainIndex;
4544
import static org.hamcrest.Matchers.allOf;
4645
import static org.hamcrest.Matchers.containsString;
46+
import static org.hamcrest.Matchers.equalTo;
4747
import static org.hamcrest.Matchers.greaterThan;
4848
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
4949
import static org.hamcrest.Matchers.hasKey;
@@ -53,7 +53,6 @@
5353
import static org.hamcrest.Matchers.nullValue;
5454

5555
public class ExplainLifecycleIT extends IlmESRestTestCase {
56-
private static final Logger logger = LogManager.getLogger(ExplainLifecycleIT.class);
5756
private static final String FAILED_STEP_RETRY_COUNT_FIELD = "failed_step_retry_count";
5857
private static final String IS_AUTO_RETRYABLE_ERROR_FIELD = "is_auto_retryable_error";
5958

@@ -320,6 +319,78 @@ public void testStepInfoPreservedOnAutoRetry() throws Exception {
320319
}, 30, TimeUnit.SECONDS);
321320
}
322321

322+
/**
323+
* Test that, when there is an ILM <code>previous_step_info</code> that has had truncation applied to it (due to it being too long),
324+
* the truncation:
325+
* <ul>
326+
* <li>doesn't break the JSON returned in <code>/{index}/_ilm/explain</code></li>
327+
* <li>truncates as expected</li>
328+
* </ul>
329+
* We test this by creating an ILM policy that rolls-over at 1 document, and an index pattern that has a non-existing rollover alias
330+
* that has a very long name (to trip the truncation due to the rollover alias name being in the <code>previous_step_info</code>).
331+
* <p>
332+
* We then index a document, wait for attempted rollover, and assert that we get valid JSON and expected truncated message
333+
* in <code>/{index}/_ilm/explain</code>.
334+
*/
335+
public void testTruncatedPreviousStepInfoDoesNotBreakExplainJson() throws Exception {
336+
final String policyName = "policy-" + randomAlphaOfLength(5).toLowerCase(Locale.ROOT);
337+
338+
final Request createPolicy = new Request("PUT", "_ilm/policy/" + policyName);
339+
createPolicy.setJsonEntity("""
340+
{
341+
"policy": {
342+
"phases": {
343+
"hot": {
344+
"actions": {
345+
"rollover": {
346+
"max_docs": 1
347+
}
348+
}
349+
}
350+
}
351+
}
352+
}
353+
""");
354+
assertOK(client().performRequest(createPolicy));
355+
356+
final String indexBase = "my-logs";
357+
final String indexName = indexBase + "-" + randomAlphaOfLength(5).toLowerCase(Locale.ROOT);
358+
final String longMissingAliasName = randomAlphanumericOfLength(LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH);
359+
360+
final Request templateRequest = new Request("PUT", "_index_template/template_" + policyName);
361+
final String templateBody = Strings.format("""
362+
{
363+
"index_patterns": ["%s-*"],
364+
"template": {
365+
"settings": {
366+
"index.lifecycle.name": "%s",
367+
"index.lifecycle.rollover_alias": "%s"
368+
}
369+
}
370+
}
371+
""", indexBase, policyName, longMissingAliasName);
372+
templateRequest.setJsonEntity(templateBody);
373+
374+
assertOK(client().performRequest(templateRequest));
375+
376+
final Request indexRequest = new Request("POST", "/" + indexName + "/_doc/1");
377+
indexRequest.setJsonEntity("{\"test\":\"value\"}");
378+
assertOK(client().performRequest(indexRequest));
379+
380+
final String expectedReason = Strings.format(
381+
"index.lifecycle.rollover_alias [%s... (~122 chars truncated)",
382+
longMissingAliasName.substring(0, longMissingAliasName.length() - 107)
383+
);
384+
final Map<String, Object> expectedStepInfo = Map.of("type", "illegal_argument_exception", "reason", expectedReason);
385+
assertBusy(() -> {
386+
final Map<String, Object> explainIndex = explainIndex(client(), indexName);
387+
388+
final String assertionMessage = "Assertion failed for the following response: " + explainIndex;
389+
final Object previousStepInfo = explainIndex.get("previous_step_info");
390+
assertThat(assertionMessage, previousStepInfo, equalTo(expectedStepInfo));
391+
}, 30, TimeUnit.SECONDS);
392+
}
393+
323394
private void assertUnmanagedIndex(Map<String, Object> explainIndexMap) {
324395
assertThat(explainIndexMap.get("managed"), is(false));
325396
assertThat(explainIndexMap.get("time_since_index_creation"), is(nullValue()));

‎x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/ilm/history/ILMHistoryItem.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -87,13 +87,7 @@ public static ILMHistoryItem failure(
8787
) {
8888
Objects.requireNonNull(error, "ILM failures require an attached exception");
8989
String fullErrorString = exceptionToString(error);
90-
String truncatedErrorString = LifecycleExecutionState.truncateWithExplanation(fullErrorString);
91-
if (truncatedErrorString.equals(fullErrorString) == false) {
92-
// Append a closing quote and closing brace to attempt to make it valid JSON.
93-
// There is no requirement that it actually be valid JSON, so this is
94-
// best-effort, but does not cause problems if it is still invalid.
95-
truncatedErrorString += "\"}";
96-
}
90+
String truncatedErrorString = LifecycleExecutionState.potentiallyTruncateLongJsonWithExplanation(fullErrorString);
9791
return new ILMHistoryItem(index, policyId, timestamp, indexAge, false, executionState, truncatedErrorString);
9892
}
9993

‎x-pack/plugin/ilm/src/test/java/org/elasticsearch/xpack/ilm/history/ILMHistoryItemTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ public void testTruncateLongError() throws IOException {
142142
"{\"type\":\"illegal_argument_exception\",\"reason\":\""
143143
// We subtract a number of characters here due to the truncation being based
144144
// on the length of the whole string, not just the "reason" part.
145-
+ longError.substring(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - 47)
145+
+ longError.substring(0, LifecycleExecutionState.MAXIMUM_STEP_INFO_STRING_LENGTH - 76)
146146
)
147147
);
148-
assertThat((String) item.get("error_details"), matchesPattern(".*\\.\\.\\. \\(\\d+ chars truncated\\).*"));
148+
assertThat((String) item.get("error_details"), matchesPattern(".*\\.\\.\\. \\(~\\d+ chars truncated\\).*"));
149149
}
150150
}
151151
}

0 commit comments

Comments
 (0)