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

Update tracing-subscriber and use tracing-tree when testing #586

Merged
merged 1 commit into from
Jan 21, 2022

Conversation

nox
Copy link
Contributor

@nox nox commented Dec 9, 2021

This makes reading the logs way easier on the eyes.

@nox
Copy link
Contributor Author

nox commented Dec 9, 2021

This removes timestamps, but durations are still shown, so I don't know if this is good enough or not.

Copy link
Collaborator

@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.

overall, this seems like a good idea, and i'm onboard with switching to the tracing-tree crate for test logging. i had a few suggestions --- in particular, i think we probably want the log output to be captured by libtest.

once that's addressed, i think this will be good to merge.

tests/h2-support/src/trace.rs Outdated Show resolved Hide resolved
tests/h2-support/src/trace.rs Outdated Show resolved Hide resolved
.with_indent_lines(true)
.with_ansi(use_colors)
.with_targets(true)
.with_indent_amount(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

take it or leave it: perhaps we want to add

Suggested change
.with_indent_amount(2);
.with_indent_amount(2)
.with_thread_ids(true)
.with_thread_names(true);

in particular, adding thread names will give us a nice way to know which test the events were logged from, since the thread's name defaults to the name of the test.

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 added it but I'm not sure this is useful: I only ever look at logs running a single test, not many of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Capture d’écran 2022-01-20 à 10 45 20

It's a bit verbose as you can see.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, we could remove it --- i suppose that since libtest is capturing the logs now, they'll also be printed out per-test. the only reason to consider having thread names here is if we have tests that spawn multiple threads and they give names to those threads...and we only have one test that spawns threads (https://github.com/hyperium/h2/search?q=thread%3A%3Aspawn).

i'm fine with removing the thread-name configuration, then!

tests/h2-support/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

okay, this looks good to me!

tests/h2-support/src/trace.rs Outdated Show resolved Hide resolved
This makes reading the logs way easier on the eyes.
@hawkw hawkw merged commit a28a39c into master Jan 21, 2022
@hawkw hawkw deleted the tracing-tree branch January 21, 2022 18:59
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.

None yet

2 participants