RFR: 8273471: Add foldmultilines to UL for stdout/err

[David Holmes]

On Wed, 8 Sep 2021 06:12:21 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

> We've introduced `foldmultilines` to UL in [JDK-8271186](https://bugs.openjdk.java.net/browse/JDK-8271186) (PR #4885 ) to replace newline characters within a multiline log event with the character sequence '' and 'n'. It helps to work log shippers.
> 
> `foldmultilines` supports file output only. It is useful if it affects to stdout/err output because many container applications would redirect logs to stdout.
> 
> `foldmultilines` will be enabled after option processing, so note that the log what happens before it isn't folded. AFAICS following logs might happen.
> They are not practical concern because the log which happens before option processing is determinative and does not have newline char (at least now), so I think we can excuse they are not folded.
> 
> 
> share/logging/logConfiguration.cpp:379:        log_error(logging)("Missing terminating quote in -Xlog option '%s'", str);
> share/logging/logConfiguration.cpp:397:    log_warning(logging)("Ignoring excess -Xlog options: "%s"", str);
> 
> share/prims/jvmtiTrace.cpp:91:      log_warning(arguments)("-XX:+TraceJVMTI specified, "
> 
> share/runtime/os.cpp:700:    log_warning(malloc, free)("os::malloc caught, " SIZE_FORMAT " bytes --> " PTR_FORMAT, size, p2i(ptr));
> share/runtime/os.cpp:752:    log_warning(malloc, free)("os::realloc caught " PTR_FORMAT, p2i(memblock));
> share/runtime/os.cpp:788:    log_warning(malloc, free)("os::free caught " PTR_FORMAT, p2i(memblock));
> 
> 
> I checked them with `$ grep -nrE '(warning|error)' * | grep -i log | grep -vwE '(cds|gc|safepoint|thread|codecache|symboltable|stringtable|jfr|jvmti|jni|container|pagesize|metaspace|exceptions|nmt|attach|class|handshake|monitorinflation)' | grep -vwE '(log_info|log_debug)'`
> 
> In addition, we might see following logs which relate to UL:
> 
> 
> [0.000s][error][logging] Error opening log file '/all-info.log': Permission denied
> [0.000s][error][logging] Invalid tag 'aaa' in log selection.
> [0.000s][error][logging] Invalid level 'aaa' in log selection.
> [0.012s][error][logging] Invalid option 'aaa' for log output (file=all-info.log).
> [0.021s][error][logging] Unable to log to file all-info.log with log file rotation: all-info.log is not a regular file
> [0.012s][error][logging] Initialization of output 'file=all-info.log' using options 'aaa=bbb' failed.

Hi Yasumasa,

Functional changes look good.

A few issues with comments and a request with the documentation change.

Thanks,
David

src/hotspot/share/logging/logConfiguration.cpp line 413:

> 411:   // (options for existing output can't be applied.)
> 412:   // StdoutLog and StderrLog are already instantiated at static initializer
> 413:   // in logFileStreamOutput.cpp.

Suggested rewording to resolve a number of grammatical issues:
// Normally options can't be used to change an existing output (parse_log_arguments() will
// report an error), and both StdoutLog and StderrLog are created by static initializers, so we have
// to process their options (e.g. foldmultilines) directly first.

src/hotspot/share/logging/logConfiguration.cpp line 417:

> 415:       strcmp("stdout", output) == 0 || strcmp("#0", output) == 0) {
> 416:     success = StdoutLog.parse_options(output_options, &ss);
> 417:     // We are no longer to need to pass output options to parse_log_arguments().

// We no longer need to pass ...

src/hotspot/share/logging/logConfiguration.cpp line 421:

> 419:   } else if (strcmp("stderr", output) == 0 || strcmp("#1", output) == 0) {
> 420:     success = StderrLog.parse_options(output_options, &ss);
> 421:     // We are no longer to need to pass output options to parse_log_arguments().

// We no longer need to pass ...

src/hotspot/share/logging/logConfiguration.cpp line 582:

> 580:   LogTagSet::describe_tagsets(out);
> 581: 
> 582:   out->print_cr("\nAvailable log output options:");

It doesn't seem right to me to have a section on output options first, but the way the rest of this section is structured makes it awkward to ordering things appropriately. Ideally, to me, we would have this order:
- Available log outputs
- Available log output options
- Additional file output options

test/hotspot/jtreg/runtime/logging/FoldMultilinesTest.java line 44:

> 42:     private static String EXCEPTION_MESSAGE = "line 1\nline 2\\nstring";
> 43:     private static String FOLDED_EXCEPTION_MESSAGE = "line 1\\nline 2\\\\nstring";
> 44:     // Windows may out "\r\n" even though UL outs "\n" only, so we need to evaluate regex with \R.

s/out/output

Though you should always use \R to get a regex match on new-lines so I'm not sure you need to say anything.

-------------

Changes requested by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5407