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.

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

Preserve grok pattern ordering and add sort option #61671

Merged
merged 5 commits into from
Sep 8, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 2 additions & 1 deletion libs/grok/src/main/java/org/elasticsearch/grok/Grok.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -288,7 +289,7 @@ private static Map<String, String> loadBuiltinPatterns() throws IOException {
"java", "junos", "linux-syslog", "maven", "mcollective-patterns", "mongodb", "nagios",
"postgresql", "rails", "redis", "ruby", "squid"
};
Map<String, String> builtinPatterns = new HashMap<>();
Map<String, String> builtinPatterns = new LinkedHashMap<>();
for (String pattern : PATTERN_NAMES) {
try(InputStream is = Grok.class.getResourceAsStream("/patterns/" + pattern)) {
loadPatterns(builtinPatterns, is);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import java.io.IOException;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;

import static org.elasticsearch.ingest.common.IngestCommonPlugin.GROK_PATTERNS;
import static org.elasticsearch.rest.RestRequest.Method.GET;
Expand All @@ -55,16 +56,31 @@ private GrokProcessorGetAction() {

public static class Request extends ActionRequest {

public Request() {}
private final boolean sorted;

public Request(boolean sorted) {
this.sorted = sorted;
}

Request(StreamInput in) throws IOException {
super(in);
this.sorted = in.readBoolean();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be guarded with version check? What will happen if we receive request from older node where there where no sorted field?

}

@Override
public ActionRequestValidationException validate() {
return null;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeBoolean(sorted);
}

public boolean sorted() {
return sorted;
}
}

public static class Response extends ActionResponse implements ToXContentObject {
Expand Down Expand Up @@ -108,11 +124,15 @@ public TransportAction(TransportService transportService, ActionFilters actionFi
@Override
protected void doExecute(Task task, Request request, ActionListener<Response> listener) {
try {
listener.onResponse(new Response(GROK_PATTERNS));
listener.onResponse(new Response(getGrokPatternsResponse(GROK_PATTERNS, request.sorted())));
} catch (Exception e) {
listener.onFailure(e);
}
}

static Map<String, String> getGrokPatternsResponse(Map<String, String> patterns, boolean sorted) {
return sorted ? new TreeMap<>(patterns) : patterns;
Copy link
Contributor

Choose a reason for hiding this comment

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

We could extract this new TreeMap<>(patterns) to a field to only sort all patterns once as they are constant. We could then skip this method completely.

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 shuffled around a few things in 9d3a31a to sort the patterns only once while retaining the ability to test the sort option. If you think it's useful, the sorted patterns member could be pulled into a singleton enum or the like for lazy loading. I was on the fence as to whether that would be particularly beneficial.

}
}

public static class RestAction extends BaseRestHandler {
Expand All @@ -129,7 +149,9 @@ public String getName() {

@Override
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) {
return channel -> client.executeLocally(INSTANCE, new Request(), new RestToXContentListener<>(channel));
boolean sorted = request.paramAsBoolean("s", false);
Request grokPatternsRequest = new Request(sorted);
return channel -> client.executeLocally(INSTANCE, grokPatternsRequest, new RestToXContentListener<>(channel));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,19 @@
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.test.ESTestCase;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.core.IsNull.nullValue;

public class GrokProcessorGetActionTests extends ESTestCase {
private static final Map<String, String> TEST_PATTERNS = Collections.singletonMap("PATTERN", "foo");
private static final Map<String, String> TEST_PATTERNS = Map.of("PATTERN2", "foo2", "PATTERN1", "foo1");

public void testRequest() throws Exception {
GrokProcessorGetAction.Request request = new GrokProcessorGetAction.Request();
GrokProcessorGetAction.Request request = new GrokProcessorGetAction.Request(false);
BytesStreamOutput out = new BytesStreamOutput();
request.writeTo(out);
StreamInput streamInput = out.bytes().streamInput();
Expand All @@ -56,15 +58,23 @@ public void testResponseSerialization() throws Exception {
assertThat(response.getGrokPatterns(), equalTo(otherResponse.getGrokPatterns()));
}

public void testResponseSorting() {
Map<String, String> sorted = GrokProcessorGetAction.TransportAction.getGrokPatternsResponse(TEST_PATTERNS, true);
List<String> sortedKeys = new ArrayList<>(TEST_PATTERNS.keySet());
Collections.sort(sortedKeys);
assertThat(new ArrayList<>(sorted.keySet()), equalTo(sortedKeys));
}

@SuppressWarnings("unchecked")
public void testResponseToXContent() throws Exception {
GrokProcessorGetAction.Response response = new GrokProcessorGetAction.Response(TEST_PATTERNS);
try (XContentBuilder builder = JsonXContent.contentBuilder()) {
response.toXContent(builder, ToXContent.EMPTY_PARAMS);
Map<String, Object> converted = XContentHelper.convertToMap(BytesReference.bytes(builder), false, builder.contentType()).v2();
Map<String, String> patterns = (Map<String, String>) converted.get("patterns");
assertThat(patterns.size(), equalTo(1));
assertThat(patterns.get("PATTERN"), equalTo("foo"));
assertThat(patterns.size(), equalTo(2));
assertThat(patterns.get("PATTERN1"), equalTo("foo1"));
assertThat(patterns.get("PATTERN2"), equalTo("foo2"));
}
}
}