Bug 157274 - Add the ability to accumulate logs for specific logging channels to help diagnose test timeouts
Summary: Add the ability to accumulate logs for specific logging channels to help diag...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 154968
  Show dependency treegraph
 
Reported: 2016-05-02 13:13 PDT by Brady Eidson
Modified: 2019-10-15 21:16 PDT (History)
9 users (show)

See Also:


Attachments
Patch (28.06 KB, patch)
2016-05-02 13:40 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (30.23 KB, patch)
2016-05-02 14:23 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (30.30 KB, patch)
2016-05-02 15:05 PDT, Brady Eidson
achristensen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-05-02 13:13:45 PDT
Add the ability to accumulate logs for specific logging channels to help diagnose test timeouts

This is the current strategy going forward to explore https://bugs.webkit.org/show_bug.cgi?id=154968
Comment 1 Brady Eidson 2016-05-02 13:15:23 PDT
This will add the WTF code as well as implement it in DRT.

WK2 (including getting it to all the processes) will come shortly after.
Comment 2 Brady Eidson 2016-05-02 13:40:44 PDT
Created attachment 277929 [details]
Patch
Comment 3 Brady Eidson 2016-05-02 14:00:41 PDT
Okay, apparently my attempt at exporting some WebCore symbols fails in release builds. Dang.
Fixing.
Comment 4 Brady Eidson 2016-05-02 14:23:23 PDT
Created attachment 277935 [details]
Patch
Comment 5 Brady Eidson 2016-05-02 14:23:49 PDT
Relying on EWS for this because it will be faster than me building release locally.
Comment 6 Brady Eidson 2016-05-02 14:31:40 PDT
The GTK build failure:

Last 500 characters of output:
./Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: error: 'initializeLoggingChannelsIfNecessary' is not a member of 'WebCore'
../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: note: suggested alternative:
../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:106:6: note:   'WebCoreTestSupport::initializeLoggingChannelsIfNecessary'

Except... that's obviously bull, because initializeLoggingChannelsIfNecessary is clearly a member of WebCore in the included header "Logging.h"
Comment 7 Brady Eidson 2016-05-02 14:43:18 PDT
(In reply to comment #6)
> The GTK build failure:
> 
> Last 500 characters of output:
> ./Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: error:
> 'initializeLoggingChannelsIfNecessary' is not a member of 'WebCore'
> ../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: note:
> suggested alternative:
> ../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:106:6: note:  
> 'WebCoreTestSupport::initializeLoggingChannelsIfNecessary'
> 
> Except... that's obviously bull, because
> initializeLoggingChannelsIfNecessary is clearly a member of WebCore in the
> included header "Logging.h"

I get the same error on Mac in a release build.
Odd.
Comment 8 Brady Eidson 2016-05-02 14:51:28 PDT
It's a compiled ifdef for logging. Of course!
Comment 9 Brady Eidson 2016-05-02 15:05:57 PDT
Created attachment 277939 [details]
Patch
Comment 10 Alex Christensen 2016-05-02 15:17:33 PDT
Comment on attachment 277939 [details]
Patch

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

> Source/WTF/wtf/Assertions.cpp:593
>  } // extern "C"
> +
> +namespace WTF {

Everything else in this file has the WTF prefix on its name.  Is there a reason you didn't want to put this in its own file?  LoggingAccumulator.cpp
Comment 11 Brady Eidson 2016-05-02 15:20:42 PDT
(In reply to comment #10)
> Comment on attachment 277939 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277939&action=review
> 
> > Source/WTF/wtf/Assertions.cpp:593
> >  } // extern "C"
> > +
> > +namespace WTF {
> 
> Everything else in this file has the WTF prefix on its name.  Is there a
> reason you didn't want to put this in its own file?  LoggingAccumulator.cpp

The only reason everything in this file is prefixed with WTF and not in the WTF namespace is because Assertions.h has to be includible from C or Obj-C code.

LoggingAccumulator.h exposes two convenience methods for the mechanism with C++ concepts, but there's no need for a .cpp. In fact adding a .cpp would separate the implementation details from where they are actively used, inside of Assertions.cpp.
Comment 12 Brady Eidson 2016-05-02 16:10:54 PDT
As I work on the WK2 part of this, I realize, this approach really only works for WK1.

In WK2 we have to message around to each process to enable the accumulation, and then message to each process for the accumulated logs.

The result is we have multiple "chunks" of logs, such as one chunk from the web process and one from the database process.

Unlike how the logging information comes to stderr, those chunks are not intertwined.

I'm going to go ahead with this patch as-is for WK1, as there will still be benefit from doing so, but revisit WK2 later.
Comment 13 Brady Eidson 2016-05-02 16:13:46 PDT
http://trac.webkit.org/changeset/200346
Comment 14 Fujii Hironori 2019-10-14 20:21:00 PDT
If a test case calls testRunner.accummulateLogsForChannel("IndexedDB"),
all subsequent IndexedDB tests dumps the log.
This is bloating DumpRenderTree log because DumpRenderTree is run with --debug-rwt-logging switch.
For example,
https://build.webkit.org/builders/Apple-Catalina-Debug-WK1-Tests/builds/70/steps/layout-test/logs/stdio

Brady, do you still need this logging function?
Can I remove this?
Or, should I add a function to turn off WTFLogChannelOnWithAccumulation flag and call it after test execution like getAndResetAccumulatedLogs?
Comment 15 Fujii Hironori 2019-10-15 21:16:07 PDT
Filed: Bug 203024 – DumpRenderTree should clear WTFLogChannelState::OnWithAccumulation state set by testRunner.accummulateLogsForChannel