Skip to content

Reduce the log in tests#9398

Merged
Jackie-Jiang merged 1 commit intoapache:masterfrom
Jackie-Jiang:test_log
Sep 14, 2022
Merged

Reduce the log in tests#9398
Jackie-Jiang merged 1 commit intoapache:masterfrom
Jackie-Jiang:test_log

Conversation

@Jackie-Jiang
Copy link
Contributor

Reduce the amount of log in tests:

  • Log in WARN level for Pinot library
  • Log in ERROR level for non-Pinot library
    Do not use async logger for tests because it can cause confusion when debugging
    Turn off the logger for KafkaConfluentSchemaRegistryAvroMessageDecoder because we intentionally inject tombstones in KafkaConfluentSchemaRegistryAvroMessageDecoderRealtimeClusterIntegrationTest which can flood the log

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Merging #9398 (4ae5b92) into master (ff2a333) will increase coverage by 6.29%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #9398      +/-   ##
============================================
+ Coverage     63.40%   69.69%   +6.29%     
- Complexity     4762     4787      +25     
============================================
  Files          1832     1885      +53     
  Lines         98146   100411    +2265     
  Branches      15020    15279     +259     
============================================
+ Hits          62231    69985    +7754     
+ Misses        31321    25462    -5859     
- Partials       4594     4964     +370     
Flag Coverage Δ
integration1 26.02% <ø> (?)
integration2 24.72% <ø> (?)
unittests1 66.92% <ø> (+<0.01%) ⬆️
unittests2 15.38% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ore/query/scheduler/resources/ResourceManager.java 84.00% <0.00%> (-12.00%) ⬇️
...he/pinot/segment/local/segment/store/IndexKey.java 65.00% <0.00%> (-5.00%) ⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java 77.77% <0.00%> (-2.78%) ⬇️
...org/apache/pinot/core/common/ObjectSerDeUtils.java 90.71% <0.00%> (-0.78%) ⬇️
...t/plugin/inputformat/json/JSONRecordExtractor.java 0.00% <0.00%> (ø)
...server/starter/helix/SegmentReloadStatusValue.java 100.00% <0.00%> (ø)
...g/apache/pinot/server/api/resources/ErrorInfo.java 0.00% <0.00%> (ø)
...t/server/api/resources/SegmentMetadataFetcher.java 56.45% <0.00%> (ø)
...pache/pinot/plugin/metrics/yammer/YammerMeter.java 55.55% <0.00%> (ø)
...pinot/server/starter/helix/HelixServerStarter.java 8.33% <0.00%> (ø)
... and 419 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

<PatternLayout>
<pattern>%d{HH:mm:ss.SSS} %c{1} - %m%n</pattern>
</PatternLayout>
<PatternLayout pattern="%d{HH:mm:ss.SSS} %p [%c{1}] [%t] %m%n"/>
Copy link
Contributor

@snleee snleee Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any kind of job that parses from the log outputs? (e.g. compatibility test) Since this is backward incompatible, it may have an impact if we ever parse the output logs from the tests.

I think that changing the pattern for test logs should be fine as long as we don't read anywhere in our testing jobs on Github action. But, we should be careful if we want to change the production log pattern (this PR seems to be touching test only).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change the production log pattern, but only the test log pattern to match the integration test setting, which gives some useful debugging info such as log level and thread.

@mcvsubbu
Copy link
Contributor

We don't "parse" the logs (in the sense that we don't have programs that run through the logs). I must admit that when I see failures, INFO logs helped much more than warning logs (that happen much later, many times hiding the root cause). We used it a lot while debugging (and reproducing) helix issues.

Ideally, if we can find a way to get all logs for a test, and discard them if the test passes, that will be best. Maybe this can be a newbie task?

Until then, I am tempted to say keep the logs.

@Jackie-Jiang
Copy link
Contributor Author

@mcvsubbu I created this PR because in the current integration test suite, we logged way too many lines and get truncated logs in CI. I even get browser crashed several times trying to download the raw log to see which test failed. IMO we should make the log proper size so that we can easily scroll to the end and find the failed tests. In most of the cases, the failure message is good enough to indicate what is going wrong.

If we can log everything for the failed test, that will be ideal, but I'm not sure how to achieve that, and we should also put a limit for the total lines of logs allowed.

@Jackie-Jiang Jackie-Jiang merged commit 92f9056 into apache:master Sep 14, 2022
@Jackie-Jiang Jackie-Jiang deleted the test_log branch September 14, 2022 21:33
@snleee
Copy link
Contributor

snleee commented Sep 14, 2022

@mcvsubbu @Jackie-Jiang Let's run this model (filtered logs) for a while and see if this makes our process more painful or not. I also faced the issue where I was not easily scrolling down to check the actual error due to too many logs and the browser slowed down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants