You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by ottobackwards <gi...@git.apache.org> on 2018/09/04 18:11:27 UTC
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
GitHub user ottobackwards opened a pull request:
https://github.com/apache/metron/pull/1184
METRON-1761, allow application of grok statement multiple times
This PR adds support for incoming messages to grok parsers that have multiple log lines.
Instead of having to split the logs before sending to metron/kafka, you could just send the logs in batches.
# todo testing
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [-] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [x] Have you written or updated unit tests and or integration tests to verify your changes?
- [-] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [-] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ottobackwards/metron grok-split
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1184.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1184
----
commit 5cb7cb2b869694ceff37c7e07e16358e4b918b2c
Author: Otto Fowler <ot...@...>
Date: 2018-09-04T10:50:38Z
METRON-1761, allow application of grok statement multiple times
----
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223836809
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineGrokParserTest.java ---
@@ -0,0 +1,146 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.metron.parsers.interfaces.MessageParserResult;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class MultiLineGrokParserTest {
+
+ /**
+ * Test that if a byte[] with multiple lines of log is passed in
+ * it will be parsed into the correct number of messages.
+ * @throws IOException if we can't read from disk
+ * @throws ParseException if we can't parse
+ */
+ @Test
+ @SuppressWarnings("unchecked")
+ public void test() throws IOException, ParseException {
--- End diff --
done
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@merrimanr @mmiklavc First pass of what we discussed
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
This looks good to me @ottobackwards. I think the only thing left is some docs telling users how to add both existing and custom parsers to take advantage of this feature.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223827440
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/bolt/ParserBolt.java ---
@@ -383,7 +408,7 @@ public void execute(Tuple tuple) {
}
}
- protected void handleError(byte[] originalMessage, Tuple tuple, Throwable ex, OutputCollector collector) {
+ protected void handleError(Object originalMessage, Tuple tuple, Throwable ex, OutputCollector collector) {
--- End diff --
Does this need to be relaxed to Object? It looks to me like byte[] is still used everywhere for original message.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223822914
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/interfaces/MessageParser.java ---
@@ -31,23 +35,41 @@
/**
* Take raw data and convert it to a list of messages.
*
- * @param rawMessage
+ * @param rawMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
List<T> parse(byte[] rawMessage);
/**
* Take raw data and convert it to an optional list of messages.
- * @param parseMessage
+ * @param parseMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
default Optional<List<T>> parseOptional(byte[] parseMessage) {
--- End diff --
Ah, left for backwards compatibility. The 2 default impls mean no need to change existing parsers.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@merrimanr any comment?
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
If you think the "every line fails" == fail, some fails = emit and log works, we can do that, but I don't know how or if we want to put things in the error stream. I need to look at it more closely. I think that solving this might be beyond this pr, with a documentation note as you state.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by lvets <gi...@git.apache.org>.
Github user lvets commented on the issue:
https://github.com/apache/metron/pull/1184
So I talked @ottobackwards into initially adding this feature 😄 The reason I asked for this improvement is that have a bunch of log files which contain a relevant entry per line. I currently have to rely on 3rdparty tools to split these logfiles into messages (where 1 log line == 1 message) before sending them to Metron. I was hoping to just ingest the log and Metron would take care of this.
To me, relying on 3rdparty tools for what I assume to be a normal use case for a SIEM seems a bit strange.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r215704742
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineGrokParserTest.java ---
@@ -0,0 +1,112 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.adrianwalker.multilinestring.Multiline;
+import org.apache.commons.io.IOUtils;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class MultiLineGrokParserTest extends GrokParserTest {
--- End diff --
Comparing this test with the base class and `SampleGrokParserTest`, there doesn't seem to be any value in having this extend the abstract `GrokParserTest` class. The `test()` method is overridden, and the other nugget of value, `compare(...)`, is not used. I think your test would be much cleaner and succinct if it was simply separated from the 1-line-per-message test.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223824193
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineGrokParserTest.java ---
@@ -0,0 +1,146 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.metron.parsers.interfaces.MessageParserResult;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class MultiLineGrokParserTest {
+
+ /**
+ * Test that if a byte[] with multiple lines of log is passed in
+ * it will be parsed into the correct number of messages.
+ * @throws IOException if we can't read from disk
+ * @throws ParseException if we can't parse
+ */
+ @Test
+ @SuppressWarnings("unchecked")
+ public void test() throws IOException, ParseException {
--- End diff --
Can you provide a descriptive name here?
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards closed the pull request at:
https://github.com/apache/metron/pull/1184
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
re-opening for input
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@mmiklavc but we don't have messages to split, we have bytes. If we where going to leave the 'parser's as single object -> single result | single exceception', ie not change the interface and not subvert validate, then we would have to introduce 'splitStrategies' at the bolt.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223836783
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineWithErrorsGrokParserTest.java ---
@@ -0,0 +1,146 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.metron.parsers.interfaces.MessageParserResult;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class MultiLineWithErrorsGrokParserTest {
+
+ /**
+ * Test that if a byte[] with multiple lines of log is passed in
+ * it will be parsed into the correct number of messages.
+ * @throws IOException if we can't read from disk
+ * @throws ParseException if we can't parse
+ */
+ @Test(expected = RuntimeException.class)
+ @SuppressWarnings("unchecked")
+ public void test() throws IOException, ParseException {
--- End diff --
done
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
Let me give it a shot, I'll document the semantics of the failure mode and we can look again
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223810558
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/GrokParser.java ---
@@ -134,26 +144,102 @@ public void init() {
@SuppressWarnings("unchecked")
@Override
public List<JSONObject> parse(byte[] rawMessage) {
+ Optional<MessageParserResult<JSONObject>> resultOptional = parseOptionalResult(rawMessage);
+ if (!resultOptional.isPresent()) {
+ return Collections.EMPTY_LIST;
+ }
+ Map<Object,Throwable> errors = resultOptional.get().getMessageThrowables();
+ if (!errors.isEmpty()) {
+ throw new RuntimeException(errors.entrySet().iterator().next().getValue());
+ }
+
+ return resultOptional.get().getMessages();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public Optional<MessageParserResult<JSONObject>> parseOptionalResult(byte[] rawMessage) {
if (grok == null) {
init();
}
+ if (multiLine) {
+ return parseMultiLine(rawMessage);
+ }
+ return parseSingleLine(rawMessage);
+ }
+
+ @SuppressWarnings("unchecked")
+ private Optional<MessageParserResult<JSONObject>> parseMultiLine(byte[] rawMessage) {
List<JSONObject> messages = new ArrayList<>();
+ Map<Object,Throwable> errors = new HashMap<>();
String originalMessage = null;
// read the incoming raw data as if it may have multiple lines of logs
// if there is only only one line, it will just get processed.
try (BufferedReader reader = new BufferedReader(new StringReader(new String(rawMessage, StandardCharsets.UTF_8)))) {
while ((originalMessage = reader.readLine()) != null) {
LOG.debug("Grok parser parsing message: {}", originalMessage);
- Match gm = grok.match(originalMessage);
- gm.captures();
- JSONObject message = new JSONObject();
- message.putAll(gm.toMap());
+ try {
+ Match gm = grok.match(originalMessage);
+ gm.captures();
+ JSONObject message = new JSONObject();
+ message.putAll(gm.toMap());
- if (message.size() == 0)
- throw new RuntimeException("Grok statement produced a null message. Original message was: "
- + originalMessage + " and the parsed message was: " + message + " . Check the pattern at: "
- + grokPath);
+ if (message.size() == 0) {
+ Throwable rte = new RuntimeException("Grok statement produced a null message. Original message was: "
+ + originalMessage + " and the parsed message was: " + message + " . Check the pattern at: "
+ + grokPath);
+ errors.put(originalMessage, rte);
+ continue;
+ }
+ message.put("original_string", originalMessage);
+ for (String timeField : timeFields) {
+ String fieldValue = (String) message.get(timeField);
+ if (fieldValue != null) {
+ message.put(timeField, toEpoch(fieldValue));
+ }
+ }
+ if (timestampField != null) {
+ message.put(Constants.Fields.TIMESTAMP.getName(), formatTimestamp(message.get(timestampField)));
+ }
+ message.remove(patternLabel);
+ postParse(message);
+ messages.add(message);
+ LOG.debug("Grok parser parsed message: {}", message);
+ } catch (Exception e) {
+ LOG.error(e.getMessage(), e);
+ errors.put(originalMessage, e);
+ }
+ }
+ } catch (IOException e) {
--- End diff --
This is specifically for exceptions thrown during reader use? The inner try/catch w/Exception appears to handle everything else parser related.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
This is great @ottobackwards, thanks for taking the time. +1 by inspection.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/1184
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223837103
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/interfaces/MessageParser.java ---
@@ -31,23 +35,41 @@
/**
* Take raw data and convert it to a list of messages.
*
- * @param rawMessage
+ * @param rawMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
List<T> parse(byte[] rawMessage);
/**
* Take raw data and convert it to an optional list of messages.
- * @param parseMessage
+ * @param parseMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
default Optional<List<T>> parseOptional(byte[] parseMessage) {
return Optional.ofNullable(parse(parseMessage));
}
+ /**
+ * Take raw data and convert it to messages. Each raw message may produce multiple messages and therefore
+ * multiple errors. A {@link MessageParserResult} is returned, which will have both the messages produced
+ * and the errors.
+ * @param parseMessage the raw bytes of the message
+ * @return Optional of {@link MessageParserResult}
+ */
+ default Optional<MessageParserResult<T>> parseOptionalResult(byte[] parseMessage) {
--- End diff --
Right, the Optional interface is the newer, so I 'built' on that approach.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223820390
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/interfaces/MessageParser.java ---
@@ -31,23 +35,41 @@
/**
* Take raw data and convert it to a list of messages.
*
- * @param rawMessage
+ * @param rawMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
List<T> parse(byte[] rawMessage);
/**
* Take raw data and convert it to an optional list of messages.
- * @param parseMessage
+ * @param parseMessage the raw bytes of the message
* @return If null is returned, this is treated as an empty list.
*/
default Optional<List<T>> parseOptional(byte[] parseMessage) {
return Optional.ofNullable(parse(parseMessage));
}
+ /**
+ * Take raw data and convert it to messages. Each raw message may produce multiple messages and therefore
+ * multiple errors. A {@link MessageParserResult} is returned, which will have both the messages produced
+ * and the errors.
+ * @param parseMessage the raw bytes of the message
+ * @return Optional of {@link MessageParserResult}
+ */
+ default Optional<MessageParserResult<T>> parseOptionalResult(byte[] parseMessage) {
--- End diff --
I had mixed feelings about this at first. There are 2 levels deep of Optional, which feels a bit like a code smell. However, our existing interface has an Optional<List<T>> which is presumably to force a contract to our clients writing custom parsers, which we have little control over. Contrast this with the unified enrichment topology where we do control the enrichment process and so we have no need to deal with Optionals. So I think this interface change is probably ok.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
I am sorry, I missed the comments on this. I will try to have something soon.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
actually, I'm just going to close this. Once I step back from "how could I do this" to look at the big picture, it doesn't seem like a good idea. This is not a field issue, more of an improvement, so there is no need to hammer something in.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@mmiklavc I looked through the validation stuff more, I think that validation is the way to go here. The grok parser will add invalid message for each exception, parser failure, and then in the validation call fail those messages. It will have to be done so that the returned message makes sense when it is sent to the error topic.
What do you think? @cestella ?
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
It think the API should be improved ideally, but that is in the future
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
Looking at the code in parserbolt, I think the only thing missing with the validate route is the original parser exception. Here's how errors are handled when validation error occurs:
```
ErrorUtils.handleError(collector, error);
```
here's how they're handled when an exception occurs during parsing:
```
} catch (Throwable ex) {
handleError(originalMessage, tuple, ex, collector);
}
```
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
So the idea would be that the JSONObject returned for the failed line ( that would be passed to handle error ) would be a new object that had the raw line, the exception type, the exception message in it.
That would all get passed through to the Error stuff. We could put anything we wanted in there.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1184
@ottobackwards Any update on this? I think it's valuable to have in.
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223833982
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/GrokParser.java ---
@@ -134,26 +144,102 @@ public void init() {
@SuppressWarnings("unchecked")
@Override
public List<JSONObject> parse(byte[] rawMessage) {
+ Optional<MessageParserResult<JSONObject>> resultOptional = parseOptionalResult(rawMessage);
+ if (!resultOptional.isPresent()) {
+ return Collections.EMPTY_LIST;
+ }
+ Map<Object,Throwable> errors = resultOptional.get().getMessageThrowables();
+ if (!errors.isEmpty()) {
+ throw new RuntimeException(errors.entrySet().iterator().next().getValue());
+ }
+
+ return resultOptional.get().getMessages();
+ }
+
+ @SuppressWarnings("unchecked")
+ @Override
+ public Optional<MessageParserResult<JSONObject>> parseOptionalResult(byte[] rawMessage) {
if (grok == null) {
init();
}
+ if (multiLine) {
+ return parseMultiLine(rawMessage);
+ }
+ return parseSingleLine(rawMessage);
+ }
+
+ @SuppressWarnings("unchecked")
+ private Optional<MessageParserResult<JSONObject>> parseMultiLine(byte[] rawMessage) {
List<JSONObject> messages = new ArrayList<>();
+ Map<Object,Throwable> errors = new HashMap<>();
String originalMessage = null;
// read the incoming raw data as if it may have multiple lines of logs
// if there is only only one line, it will just get processed.
try (BufferedReader reader = new BufferedReader(new StringReader(new String(rawMessage, StandardCharsets.UTF_8)))) {
while ((originalMessage = reader.readLine()) != null) {
LOG.debug("Grok parser parsing message: {}", originalMessage);
- Match gm = grok.match(originalMessage);
- gm.captures();
- JSONObject message = new JSONObject();
- message.putAll(gm.toMap());
+ try {
+ Match gm = grok.match(originalMessage);
+ gm.captures();
+ JSONObject message = new JSONObject();
+ message.putAll(gm.toMap());
- if (message.size() == 0)
- throw new RuntimeException("Grok statement produced a null message. Original message was: "
- + originalMessage + " and the parsed message was: " + message + " . Check the pattern at: "
- + grokPath);
+ if (message.size() == 0) {
+ Throwable rte = new RuntimeException("Grok statement produced a null message. Original message was: "
+ + originalMessage + " and the parsed message was: " + message + " . Check the pattern at: "
+ + grokPath);
+ errors.put(originalMessage, rte);
+ continue;
+ }
+ message.put("original_string", originalMessage);
+ for (String timeField : timeFields) {
+ String fieldValue = (String) message.get(timeField);
+ if (fieldValue != null) {
+ message.put(timeField, toEpoch(fieldValue));
+ }
+ }
+ if (timestampField != null) {
+ message.put(Constants.Fields.TIMESTAMP.getName(), formatTimestamp(message.get(timestampField)));
+ }
+ message.remove(patternLabel);
+ postParse(message);
+ messages.add(message);
+ LOG.debug("Grok parser parsed message: {}", message);
+ } catch (Exception e) {
+ LOG.error(e.getMessage(), e);
+ errors.put(originalMessage, e);
+ }
+ }
+ } catch (IOException e) {
--- End diff --
That is right
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1184
+1 from me too. I will merge this into https://github.com/apache/metron/pull/1213 once it's in master and we can continue moving forward there. Parsing is getting an upgrade!
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@mmiklavc Can you take a look at the parser.validate() stuff in the bolt? Maybe the answer is put a dummy invalid record in there and fail validation for each parse failure?
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r215710648
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineGrokParserTest.java ---
@@ -0,0 +1,112 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.adrianwalker.multilinestring.Multiline;
+import org.apache.commons.io.IOUtils;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class MultiLineGrokParserTest extends GrokParserTest {
--- End diff --
Also, can you add some tests around parsing failure scenarios in the batch case?
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
GitHub user ottobackwards reopened a pull request:
https://github.com/apache/metron/pull/1184
METRON-1761, allow application of grok statement multiple times
This PR adds support for incoming messages to grok parsers that have multiple log lines.
Instead of having to split the logs before sending to metron/kafka, you could just send the logs in batches.
# todo testing
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [-] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [x] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [x] Have you written or updated unit tests and or integration tests to verify your changes?
- [-] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [-] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ottobackwards/metron grok-split
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1184.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1184
----
commit 498313a8c12bcc15c8a179f5a97afff1d673d0b2
Author: Otto Fowler <ot...@...>
Date: 2018-09-04T10:50:38Z
METRON-1761, allow application of grok statement multiple times
commit 05b734cf18be120cbabea554a0d84bb0164193c9
Author: Otto Fowler <ot...@...>
Date: 2018-09-28T14:17:41Z
Merge remote-tracking branch 'apache/master' into grok-split
commit c2b3bb88d2a06e5cde39fd90a87f92207906eac4
Author: Otto Fowler <ot...@...>
Date: 2018-09-28T15:40:37Z
per review, do not require derivation
commit 91764c924a7a85e4f8146105857234ad7b046c72
Author: Otto Fowler <ot...@...>
Date: 2018-10-02T13:12:56Z
Merge remote-tracking branch 'apache/master' into grok-split
----
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223836515
--- Diff: metron-platform/metron-parsers/src/main/java/org/apache/metron/parsers/bolt/ParserBolt.java ---
@@ -383,7 +408,7 @@ public void execute(Tuple tuple) {
}
}
- protected void handleError(byte[] originalMessage, Tuple tuple, Throwable ex, OutputCollector collector) {
+ protected void handleError(Object originalMessage, Tuple tuple, Throwable ex, OutputCollector collector) {
--- End diff --
Sorry, that was from an earlier rev. where I was using that.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@mmiklavc wrt failing the whole message or some sort of partial failure scheme. I don't like failing multiples if we _can_ parse some lines, but I don't see a good way to cleanly handle it. Or we need to talk it through.
We could track exceptions and lines processed.... and if we got an exception for every line fail the file, else log the execeptions and a message about only parsing x of y lines and pass back a partial message list.
Thoughts?
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
@ottobackwards Yeah, understood about the message vs raw bytes. It seems like there are 2 options here - use validate or modify the message parser interface to return a List of something other than a `List<JSONObject>`. If you were to go the validate route, how would you modify and return the messages that failed to parse so that validate can handle them? I'd expect them to be indexed to the error topic in the same style as the single message parsers.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
@ottobackwards Yeah, I took a look through the ParserBolt and the MessageParser interface. We process a raw byte array. A clean way of doing that isn't immediately obvious to me either. Perhaps these semantics could be documented for the parser? Minimally, I think we should describe the 2 cases I outlined above.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
Closing this pr. I will create a jira for api improvement
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
@ottobackwards That sounds reasonable to me. One thing I would definitely add to the documentation for this is some comment about how it changes processing semantics and might require additional tuning. Since we now have batch sizing specified for the writers to Kafka, there may be a little bit of tweaking there, but I'm just not sure.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1184
I prefer @mmiklavc's suggestion several comments back: return a List of something other than a List<JSONObject>. Could we use a wrapper that can contain results and errors? The calling class could then determine the best way to handle errors without having to fail the whole batch.
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
@mmiklavc please see latest commit
---
[GitHub] metron pull request #1184: METRON-1761, allow application of grok statement ...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/1184#discussion_r223824289
--- Diff: metron-platform/metron-parsers/src/test/java/org/apache/metron/parsers/MultiLineWithErrorsGrokParserTest.java ---
@@ -0,0 +1,146 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.metron.parsers;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.metron.parsers.interfaces.MessageParserResult;
+import org.json.simple.JSONObject;
+import org.json.simple.parser.JSONParser;
+import org.json.simple.parser.ParseException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public class MultiLineWithErrorsGrokParserTest {
+
+ /**
+ * Test that if a byte[] with multiple lines of log is passed in
+ * it will be parsed into the correct number of messages.
+ * @throws IOException if we can't read from disk
+ * @throws ParseException if we can't parse
+ */
+ @Test(expected = RuntimeException.class)
+ @SuppressWarnings("unchecked")
+ public void test() throws IOException, ParseException {
--- End diff --
Can you provide a descriptive name here?
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/1184
If it turns out that this is just such a duck tape job, we can always close the PR and open a jira for the new api
---
[GitHub] metron issue #1184: METRON-1761, allow application of grok statement multipl...
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/1184
@ottobackwards I see what you're saying. It looks like that could definitely work. Thinking out loud here, but might that conflate the semantics of our validation a bit? Validate currently does things like ensure that a timestamp exists on the message, though I don't see why we couldn't expand it to validations outside of our global Metron context.
One class that might be worth checking out is the unified enrichment topology. This was changed to include a parallel enricher that handles errors and message results in an EnrichmentResult class.
1. https://github.com/apache/metron/blob/master/metron-platform/metron-enrichment/src/main/java/org/apache/metron/enrichment/bolt/UnifiedEnrichmentBolt.java#L270
2. https://github.com/apache/metron/blob/master/metron-platform/metron-enrichment/src/main/java/org/apache/metron/enrichment/parallel/ParallelEnricher.java#L63
It looks to me like there might be some possible collaboration opportunity and overlap with the work you're doing here and the work @merrimanr is doing on this PR - https://github.com/apache/metron/pull/1213#pullrequestreview-161248142
I'm just wondering if we might be able to kill 2 birds with one stone. We probably don't want to change the MessageParser interface, but maybe we can manage the bulk processing through a more generalized bridge between the ParserBolt and parser implementations. I haven't dug too deep into implementation feasibility, but it seems worth considering.
---