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
8271186: Add UL option to replace newline char #4885
Changes from 7 commits
af24c9f
358fbe1
189dd3b
f521450
03af43f
267ebd0
55ee9cf
19bc58c
bb3f236
64122a4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -195,7 +195,16 @@ bool LogFileOutput::parse_options(const char* options, outputStream* errstream) | ||
char* value_str = equals_pos + 1; | ||
*equals_pos = '\0'; | ||
|
||
if (strcmp(FileCountOptionKey, key) == 0) { | ||
if (strcmp(FoldMultilinesOptionKey, key) == 0) { | ||
// We need to pass <key>=<value> style option to LogFileStreamOutput::initialize(). | ||
// Thus we restore '=' temporally. | ||
*equals_pos = '='; | ||
success = LogFileStreamOutput::initialize(pos, errstream); | ||
*equals_pos = '\0'; | ||
if (!success) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the code can be simplified by parsing the value here:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, your concern is I suppose this would be useful if we have support for sockets in the future, like
However, I don't think your current implementation has the right abstraction -- I think we should defer the design of such an abstraction until we need it. Ideally, For the time being, it's easier to parse all the file output options in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, I think we can (should) improve more. Its improvement is not for Anyway I moved |
||
break; | ||
} | ||
} else if (strcmp(FileCountOptionKey, key) == 0) { | ||
size_t value = parse_value(value_str); | ||
if (value > MaxRotationFileCount) { | ||
errstream->print_cr("Invalid option: %s must be in range [0, %u]", | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -4418,8 +4418,13 @@ selected. | ||
\f[I]output\-options\f[R] is | ||
.RS | ||
.PP | ||
\f[CB]filecount=\f[R]\f[I]file\-count\f[R] \f[CB]filesize=\f[R]\f[I]file size | ||
with optional K, M or G suffix\f[R] | ||
\f[CB]filecount=\f[R]\f[I]file\-count\f[R] \f[CB]filesize=\f[R]\f[I]<file size | ||
with optional K, M or G suffix>\f[R] \f[CB]foldmultilines=\f[R]\f[I]<true|false>\f[R] | ||
.RE | ||
.PP | ||
\f[I]foldmultilines\f[R] enables to replace newline characters within | ||
a multiline log event with the character sequence '\\' and 'n'. | ||
Note that it works on file output only. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be expanded similar to the help info. But you can elaborate on the character encoding limitation as per the CSR request. |
||
.RE | ||
.RE | ||
.SS Default Configuration | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
/* | ||
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. | ||
* Copyright (c) 2021 NTT DATA. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/* | ||
* @test | ||
* @bug 8271186 | ||
* @library /test/lib | ||
* @run driver FoldMultilinesTest | ||
*/ | ||
|
||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import jdk.test.lib.process.OutputAnalyzer; | ||
import jdk.test.lib.process.ProcessTools; | ||
|
||
public class FoldMultilinesTest { | ||
|
||
private static Path EXCEPTION_LOG_FILE = Path.of("exceptions.log"); | ||
private static String XLOG_BASE = "-Xlog:exceptions=info:file=" + EXCEPTION_LOG_FILE.toString(); | ||
|
||
private static void analyzeFoldMultilinesOn(ProcessBuilder pb) throws Exception { | ||
OutputAnalyzer output = new OutputAnalyzer(pb.start()); | ||
output.shouldHaveExitValue(0); | ||
|
||
String logs = Files.readString(EXCEPTION_LOG_FILE); | ||
if (!logs.contains("line 1\\nline 2\\\\nstring")) { | ||
throw new RuntimeException("foldmultilines=true did not work."); | ||
} | ||
} | ||
|
||
private static void analyzeFoldMultilinesOff(ProcessBuilder pb) throws Exception { | ||
OutputAnalyzer output = new OutputAnalyzer(pb.start()); | ||
output.shouldHaveExitValue(0); | ||
|
||
String logs = Files.readString(EXCEPTION_LOG_FILE); | ||
if (!logs.contains("line 1" + System.lineSeparator() + "line 2\\nstring")) { | ||
throw new RuntimeException("foldmultilines=false did not work."); | ||
} | ||
} | ||
|
||
private static void analyzeFoldMultilinesInvalid(ProcessBuilder pb) throws Exception { | ||
OutputAnalyzer output = new OutputAnalyzer(pb.start()); | ||
output.shouldContain("Invalid option 'invalid' for foldmultilines."); | ||
output.shouldNotHaveExitValue(0); | ||
} | ||
|
||
public static void main(String[] args) throws Exception { | ||
String Xlog; | ||
ProcessBuilder pb; | ||
|
||
Xlog = XLOG_BASE + "::foldmultilines=true"; | ||
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName()); | ||
analyzeFoldMultilinesOn(pb); | ||
|
||
Xlog = XLOG_BASE + "::foldmultilines=false"; | ||
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName()); | ||
analyzeFoldMultilinesOff(pb); | ||
|
||
Xlog = XLOG_BASE + "::foldmultilines=invalid"; | ||
pb = ProcessTools.createJavaProcessBuilder(Xlog, InternalClass.class.getName()); | ||
analyzeFoldMultilinesInvalid(pb); | ||
} | ||
|
||
public static class InternalClass { | ||
public static void main(String[] args) { | ||
try { | ||
throw new RuntimeException("line 1\nline 2\\nstring"); | ||
} catch (Exception e) { | ||
// Do nothing to return exit code 0 | ||
} | ||
} | ||
} | ||
|
||
} |
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.
How about this message?
I think there's no need to say "it works on file output only" because this is under the heading of "Additional output-options for file outputs".
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.
We are not "escaping" newline characters here. We went through great pains in the CSR request to describe the actual process accurately. We are replacing newlines with the sequence
\
andn
. We also have to replace\
with the sequence\\
so that the conversion can be reversed by a parser. Saying all this in the help output is awkward but necessary. We also need to warn that this may not be safe with character encodings other than UTF-8. So something like:If set to true, a log event that consists of multiple lines will be folded into a single line by replacing newline characters with the sequence '\' and 'n' in the output. Existing single backslash characters will also be replaced with a sequence of two backslashes so that the conversion can be reversed. This option is safe to use with UTF-8 character encodings, but other encodings may not work.
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.
Ok, this version is much better than mine :-)