Skip to content
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

fix(test) switch zebrad to a non-blocking tracing logger #5032

Merged
merged 17 commits into from
Sep 7, 2022

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Sep 1, 2022

Motivation

We want to switch zebrad to a non-blocking tracing logger to use a separate thread when writing tracing logs to stdout so that our tests won't deadlock.

Fixes #4834.

Solution

  • Provide tracing FmtSubscriber (or fmt::Layer when tokio-console feature is enabled) with a lossy non-blocking writer to use instead of stdout

Review

Anyone can review this PR

Reviewer Checklist

  • Tests for Expected Behaviour

@arya2 arya2 requested review from a team as code owners September 1, 2022 18:35
@arya2 arya2 requested review from oxarbitrage and removed request for a team September 1, 2022 18:35
@codecov
Copy link

codecov bot commented Sep 1, 2022

Codecov Report

Merging #5032 (3e203d7) into main (ceff590) will increase coverage by 0.06%.
The diff coverage is 41.93%.

@@            Coverage Diff             @@
##             main    #5032      +/-   ##
==========================================
+ Coverage   79.27%   79.34%   +0.06%     
==========================================
  Files         310      310              
  Lines       38907    39108     +201     
==========================================
+ Hits        30843    31029     +186     
- Misses       8064     8079      +15     

@arya2 arya2 closed this Sep 1, 2022
@arya2 arya2 reopened this Sep 1, 2022
@teor2345 teor2345 added C-bug Category: This is a bug P-Medium ⚡ A-diagnostics Area: Diagnosing issues or monitoring performance labels Sep 1, 2022
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks great!

I just want to double-check a few edge cases. Let me know if you have any questions, or you'd like me to help with the code changes.

zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
zebrad/src/components/tracing/component.rs Outdated Show resolved Hide resolved
zebrad/src/components/tracing/component.rs Show resolved Hide resolved
@arya2 arya2 marked this pull request as draft September 2, 2022 02:31
@arya2 arya2 force-pushed the bug-non-blocking-logger branch from 6d2d118 to 9a2101d Compare September 2, 2022 20:45
@arya2 arya2 requested a review from teor2345 September 2, 2022 21:09
@teor2345 teor2345 removed the request for review from oxarbitrage September 5, 2022 00:26
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Sep 5, 2022

I noticed that some tests were failing, so I pushed a commit that shows the error they are failing with.

@arya2 arya2 force-pushed the bug-non-blocking-logger branch from 1194af9 to 590bb91 Compare September 5, 2022 18:43
@arya2 arya2 marked this pull request as ready for review September 5, 2022 18:57
@arya2 arya2 requested a review from teor2345 September 5, 2022 18:58
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

It looks like two other tests are failing with these changes, but only on macOS:

  • restart_stop_at_height
  • sync_one_checkpoint_mainnet

These are the tests that exit() abruptly, and don't drop the worker guard.

Here are some things to try:

  • make the test-only state code sleep for a few seconds before calling exit()
  • make the test-only state code panic instead of exiting (this could be complicated, I'm not sure if we propagate panics correctly from that far inside the state)

zebrad/src/components/tracing/component.rs Show resolved Hide resolved
zebrad/src/components/tracing/component.rs Show resolved Hide resolved
@teor2345
Copy link
Contributor

teor2345 commented Sep 6, 2022

Here are some things to try:

If we can't fix it quickly, it's ok to disable these tests on macOS. It's a tier 2 platform, so we don't guarantee test coverage.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing those tests, let's go!

mergify bot added a commit that referenced this pull request Sep 7, 2022
@mergify mergify bot merged commit d9fae6e into main Sep 7, 2022
@mergify mergify bot deleted the bug-non-blocking-logger branch September 7, 2022 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Diagnosing issues or monitoring performance C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch zebrad to a non-blocking tracing logger
2 participants