8000 Tracing appender tests by thekeys93 · Pull Request #678 · tokio-rs/tracing · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Tracing appender tests #678

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.

Sign up for GitHub

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

Jump to bottom
8000
Merged
merged 17 commits into from
May 4, 2020
Merged

Tracing appender tests #678

merged 17 commits into from
May 4, 2020

Conversation

thekeys93
Copy link
Contributor

Motivation

Adding tests for PR #673

davidbarsky
davidbarsky requested changes Apr 17, 2020
View reviewed changes
@davidbarsky
Copy link
Member

Few additional notes:

@thekeys93
Copy link
Contributor Author
thekeys93 commented Apr 17, 2020 •
edited
Loading

Few additional notes:

Clippy doesn't catch the .bytes() warnings locally for me unfortunantely, so I have to rely on the clippy checker here.

I need to also find a way to make sure cargo fmt is run before any commit as I sometimes forget.

Edit: Realized that I needed to run cargo clippy --tests for it inform me of clippy warnings.

hawkw
hawkw requested changes Apr 17, 2020
View reviewed changes
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

I want to go over the multithreaded test in more detail, but I left some preliminary notes.

hawkw
hawkw requested changes Apr 17, 2020
View reviewed changes
@thekeys93
Copy link
Contributor Author
thekeys93 commented Apr 20, 2020 •
edited
Loading

Looks like one of the tests hangs, logs_dropped_if_lossy will look to see what is causing that to happen.

Edit:
Looks like for the case when non blocking is lossy, I must drop non_blocking at the end of the test otherwise it wil hang some times.

davidbarsky
davidbarsky approved these changes Apr 20, 2020
View reviewed changes
@hawkw
8000 Copy link
Member
hawkw commented Apr 28, 2020

CI failures look like crates.io was returning 500s for a bit; I restarted them.

@thekeys93 thekeys93 requested a review from a team as a code owner April 30, 2020 17:45
@thekeys93
Copy link
Contributor Author

Rebased

davidbarsky
davidbarsky reviewed Apr 30, 2020
View reviewed changes
tracing-appender/src/lib.rs Outdated
Comment on lines 139 to 144

#[derive(Debug)]
pub(crate) enum Msg {
Line(Vec<u8>),
Shutdown,
}
8000 Copy link
Member
@davidbarsky davidbarsky Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I forgot about this: dropping a sender is equivalent to a shutdown notice.

(I'm not treating this as public, as this is not in the public API and we can tidy up later.)

@hawkw
Copy link
Member
hawkw commented Apr 30, 2020

I'd really prefer it if the change that fixes the bug could land in a separate PR from the change that adds tests, so that the Git history is clearer — @zekisherif, would you mind moving that into a separate branch?

@thekeys93
Copy link
Contributor Author

I'd really prefer it if the change that fixes the bug could land in a separate PR from the change that adds tests, so that the Git history is clearer — @zekisherif, would you mind moving that into a separate branch?

Sure I can do that.

@thekeys93
Copy link
Contributor Author

Once this is merged in, I'll raise a PR for the bug fix

@hawkw
Copy link
Member
hawkw commented Apr 30, 2020

Once this is merged in, I'll raise a PR for the bug fix

I think we should merge the bugfix first to avoid having a broken build on master.

@thekeys93 thekeys93 mentioned this pull request Apr 30, 2020
Merged
hawkw pushed a commit that referenced this pull request Apr 30, 2020
@thekeys93
Appender: Use channel to signal shutdown of worker thread (#701)
f93ba2e 
## Motivation

Fixes a race condition which occurs on dropping of `WorkerGuard`. If the
worker thread missed seeing the shutdown signal in time, it would end up
blocking on trying to call `recv()` on the crossbeam channel and block
indefinitely.

This bug was identified in #678 

## Solution

Signal that the worker should stop by sending a `ShutDown` message
through the crossbeam channel.

Co-authored-by: Zeki Sherif <zekshi@amazon.com>
@thekeys93 thekeys93 requested a review from hawkw May 1, 2020 14:42
@thekeys93 thekeys93 mentioned this pull request May 4, 2020
Merged
hawkw
hawkw approved these changes May 4, 2020
View reviewed changes
Copy link
Member
@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

A couple nits, but this looks good.

@hawkw hawkw merged commit 750b031 into tokio-rs:master May 4, 2020
hawkw added a commit that referenced this pull request May 5, 2020
@thekeys93 @hawkw
appender: prepare tracing-appender for release (#705)
f8b52bd 
Following the release process for
https://github.com/tokio-rs/tracing/blob/master/CONTRIBUTING.md and
these changes are the only things left to commit before release.

I'd like to hopefully get this released today so we can start using this
crate internally.

Note: I need to get PR #703 and PR #678 merged before release.

Co-authored-by: Zeki Sherif <zekshi@amazon.com>
Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@salewski salewski mentioned this pull request Nov 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidbarsky davidbarsky davidbarsky approved these changes

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thekeys93 @davidbarsky @hawkw
0