Bug 230725 – Add flag to enabling release log and implement stderr logging by default

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Basuke Suzuki]

Even if there's no modern logging system, dumping out those logs to stdout/stderr helps a lot on development.

[Basuke Suzuki]

Created attachment 439652 [details]
PATCH

[Radar WebKit Bug Importer]

<rdar://problem/83740529>

[Basuke Suzuki]

Created attachment 439808 [details]
PATCH

[Fujii Hironori]

Comment on attachment 439808 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review

> Source/WTF/wtf/Assertions.h:604
> +#define RELEASE_LOG_FP stdout

It's not a good idea to output logs to stdout. It breaks command line application, DumpRenderTree and WebKitTestRunner.

> Source/WTF/wtf/Assertions.h:606
> +#define RELEASE_LOG_FP stderr

Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable all log channels by setting log env vars "all".

> Source/WTF/wtf/Logger.h:354
>          sd_journal_send_with_location(fileString.utf8().data(), lineString.utf8().data(), function, "WEBKIT_SUBSYSTEM=%s", channel.subsystem, "WEBKIT_CHANNEL=%s", channel.name, "MESSAGE=%s", logMessage.utf8().data(), nullptr);

What about the idea implementing a compatible sd_journal_send_with_location for your platform.

[Basuke Suzuki]

> It's not a good idea to output logs to stdout. It breaks command line
> application, DumpRenderTree and WebKitTestRunner.

Well, I don't actually need stdout logging, but I thought it's nice to have option. About the DRT/WTR, stderr output also breaks the test result, doesn't it? We can run tests by building different binary for testing.


> Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable
> all log channels by setting log env vars "all".

I don't understand that. I need release log and that is disable without enabling os_log or jounald.


> What about the idea implementing a compatible sd_journal_send_with_location
> for your platform.

That was just too much for simple logging with stderr. Those extra info will be dump out when verbose logging.

[Fujii Hironori]

(In reply to Basuke Suzuki from comment #5)
> Well, I don't actually need stdout logging, but I thought it's nice to have
> option. About the DRT/WTR, stderr output also breaks the test result,
> doesn't it? We can run tests by building different binary for testing.

The loggs in stderr don't make the test fail. But, if a test fail, run-webkit-tests show the stderr for the hint of debugging.
For example, in the following test result, http/tests/xmlhttprequest/onload-progressevent-attributes.html failed and had a stderr message.
https://build.webkit.org/results/WinCairo-64-bit-WKL-Release-Tests/r282824%20(2701)/results.html


> > Why do you need LOG_STDERR? WebKit logs are output to stderr. You can enable
> > all log channels by setting log env vars "all".
> 
> I don't understand that. I need release log and that is disable without
> enabling os_log or jounald.

Oh, I misunderstood.

> > What about the idea implementing a compatible sd_journal_send_with_location
> > for your platform.
> 
> That was just too much for simple logging with stderr. Those extra info will
> be dump out when verbose logging.

Got it.

[Fujii Hironori]

Comment on attachment 439808 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review

> ChangeLog:3
> +        [PlayStation] Enable release logging to stdout or stderr

This patch is almost platform independent change. Darin asked me not to use the platform prefix for such platform independent changes, for example [PlayStation] and [WinCairo].

[Fujii Hironori]

How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and USE(JOURNALD) are not true, use stderr.

[Michael Catanzaro]

(In reply to Fujii Hironori from comment #8)
> How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and
> USE(JOURNALD) are not true, use stderr.

That sounds good to me. Will result in simpler #if #elifs too. The logs are all disabled by default anyway: you need to opt-in using a confusing environment variable that I can never figure out how to use.

[Michael Catanzaro]

Comment on attachment 439808 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review

> Source/WTF/wtf/Assertions.h:600
> +#error "Cannot unable both USE_LOG_STDOUT and USE_LOG_STDERR"

"Cannot use both"

>> Source/WTF/wtf/Logger.h:354
>>          sd_journal_send_with_location(fileString.utf8().data(), lineString.utf8().data(), function, "WEBKIT_SUBSYSTEM=%s", channel.subsystem, "WEBKIT_CHANNEL=%s", channel.name, "MESSAGE=%s", logMessage.utf8().data(), nullptr);
> 
> What about the idea implementing a compatible sd_journal_send_with_location for your platform.

Seems like overkill?

[Basuke Suzuki]

(In reply to Fujii Hironori from comment #6)
> The loggs in stderr don't make the test fail. But, if a test fail,
> run-webkit-tests show the stderr for the hint of debugging.
> For example, in the following test result,
> http/tests/xmlhttprequest/onload-progressevent-attributes.html failed and
> had a stderr message.
> https://build.webkit.org/results/WinCairo-64-bit-WKL-Release-Tests/
> r282824%20(2701)/results.html

Ah, make sense. That was one of my use case to use stdout so I agree to remove it for simplicity.

[Basuke Suzuki]

Comment on attachment 439808 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=439808&action=review

>> ChangeLog:3
>> +        [PlayStation] Enable release logging to stdout or stderr
> 
> This patch is almost platform independent change. Darin asked me not to use the platform prefix for such platform independent changes, for example [PlayStation] and [WinCairo].

Right. Will remove the prefix.

>> Source/WTF/wtf/Assertions.h:600
>> +#error "Cannot unable both USE_LOG_STDOUT and USE_LOG_STDERR"
> 
> "Cannot use both"

Thanks.

[Basuke Suzuki]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Fujii Hironori from comment #8)
> > How do you think an idea always enabling RELEASE_LOG, and if USE(OS_LOG) and
> > USE(JOURNALD) are not true, use stderr.
> 
> That sounds good to me. Will result in simpler #if #elifs too. The logs are
> all disabled by default anyway: you need to opt-in using a confusing
> environment variable that I can never figure out how to use.

That is kind of a big change for default configuration. There's tons of code omitted from runtime binary when not RELEASE_LOG_DISABLED. They are just there sitting silently without doing almost nothing. If we do this change, we should introduce ENABLE_RELEASE_LOG as other features do and make it completely opt-in feature. At that time, I agree with the default choice of logging. How about this?

[Michael Catanzaro]

I assumed it would not print anything unless special environment variables are enabled. I agree it should not print to stderr by default.

[Basuke Suzuki]

Created attachment 439895 [details]
PATCH

[Basuke Suzuki]

Created attachment 439898 [details]
PATCH

[Basuke Suzuki]

This is the shape including the points you guys reviewed. Removing USE_LOG_STDERR and added global flag ENABLE_RELEASE_LOG. Actual checking in the entire code base is untouched, still RELEASE_LOG_DISABLED is used. ENABLE_RELEASE_LOG is used to define that value at the beginning of Assertions.h.

[Michael Catanzaro]

Comment on attachment 439898 [details]
PATCH

A follow-up patch could replace RELEASE_LOG_DISABLED with ENABLE(RELEASE_LOG).

[Basuke Suzuki]

Comment on attachment 439898 [details]
PATCH

Thanks. I'll create a ticket for that.

[EWS]

Committed r283469 (242446@main): <https://commits.webkit.org/242446@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439898 [details].