-
Notifications
You must be signed in to change notification settings - Fork 407
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
Add option to split log messages on newlines - fixes #404 #413
Add option to split log messages on newlines - fixes #404 #413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
In addition to addressing my inline comments, please update the README.md with documentation for your change. Specifically, the message provider description/properties needs to be updated here, and a new section for "Splitting Message into an Array" could probably be added after the "Customizing Timestamp" section.
@@ -28,7 +28,9 @@ | |||
|
|||
@Override | |||
protected CompositeJsonFormatter<ILoggingEvent> createFormatter() { | |||
return new LogstashFormatter(this); | |||
LogstashFormatter formatter = new LogstashFormatter(this); | |||
formatter.setLineSeparator(getLineSeparator()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. The Encoder's lineSeparator property is the string to output between events. Coupling the separator to output between events with the regex to use to split the message string does not seem ideal. I think the regex to use to split the message string should be separate from the separator string to use between events.
public String getLineSeparator() { | ||
return messageProvider.getLineSeparator(); | ||
} | ||
|
||
public void setLineSeparator(String lineSeparator) { | ||
messageProvider.setLineSeparator(lineSeparator); | ||
} | ||
|
||
public boolean isWriteMultiLineMessage() { | ||
return messageProvider.isWriteMultiLineMessage(); | ||
} | ||
|
||
public void setWriteMultiLineMessage(boolean writeMultiLineMessage) { | ||
messageProvider.setWriteMultiLineMessage(writeMultiLineMessage); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear that the "lineSeparator" is only used as a regex to split the message into an array if and only if "writeMultiLineMessage" is true.
Additionally, the term "writeMultiLineMessage" does not convey that the message field will be written as an array. A message with embedded line separators could also be considered a "multi line message".
Perhaps, we can combine these two fields into a single String field named "messageSplitRegex". When null/empty, the message will not be split, and will be written as a JSON string. When non-null, the message will be split, and written as a JSON array.
private Pattern lineSeparatorPattern = null; | ||
private boolean writeMultiLineMessage = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as LogstashFormatter regarding naming of these fields. Combine into a single "messageSplitRegex" field.
Also add some javadoc for this field.
@philsttr, thanks for the review. I'll make the changes. I can use the existing |
I just had another thought. Do we really need regexes for splitting the message? regexes can be computationally expensive. I wonder if just searching for specific strings would be faster, maybe just using a StringTokenizer. |
A couple of problems with
Also note the Javadoc comment on
Both of those are regex-based. |
Sounds good. It's a shame there isn't an easy way to split a string that doesn't involve regexes. |
The Apache Commons StringTokenizer might be worth exploring, but it adds a new dependency on |
Just use a regex. I'd rather not introduce another dependency. |
@philsttr, all right, looks like I've addressed all the review comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful!
This PR adds an option to output the
"message"
field as a JSON array, instead of a JSON string.For example, with either of these logging statements:
The output is consistent:
This provides an unambiguous parsing format on the target system instead of relying on the source platform's newline representation.