backport several non-breaking changes to v0.1.x #1139
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Motivation At Netlify we recently introduced native Rust support in the build system: netlify/build-image#477 ## Solution This PR cleans up the Netlify build config to use a more straight-forward way of building rust docs. This also introduces a workaround for netlify/build-image#505 ## Kudos We're big fans of the `tracing` crate at Netlify and using it for many new systems recently. Very happy we can give something back! Closes #1130
…nv_logger compat) (#1126) Hi Folks, This PR is about behavior compatibility with the `env_logger` and `log` crates. There are references in the `tracing-subscriber` docs noting some level of partial compatibility with `env_logger`, but it is not clear to me the extent to which that is a priority. If the intention is to keep the projects close in behavior where there is overlap in the representations of logging directive strings, then this PR is a step toward better compatibility. On the other hand, if such compatibility is more of a short-term nice-to-have than a long-term objective, then this PR might be a step in the wrong direction. If so, please feel free to reject it. I happened to notice the behavior difference (described below) while working on something else, and just thought I'd bring it up for discussion. On the *other* other hand, it is clear that some significant effort *has* been expended to have compatibly parsed logging directive strings. Which leads me to read the regex code modified in the second commit of this PR as possibly introducing a level of compatibility that was deliberately omitted; the existing regex was clearly structured to accept *only* all uppercase OR *only* all lowercase log level names. So I'm getting mixed signals :-) In the end, regardless of the specific outcome of this PR, understanding the degree to which `env_logger` compatibility is wanted would be useful to know in general. For my own use, `env_logger` compatibility is not something I need. ## Motivation Logging directive strings parsed to create `tracing_subscriber::filter::env::Directive`s are currently accepted as all-lower-case or all-upper-case representations of the log level names (like "info" and "INFO"), but mixed case representation (like "Info", "iNfo", and "infO") are rejected. This behavior is divergent with that of the `env_logger` crate, which accepts the mixed case names. The `env_logger` crate gets the behavior of parsing mixed case log level names from the underlying `log` crate, so there may be an element of user expectations involved in that regard, too, with `log` users expecting that case-insensitive log level names will be accepted. Accepting mixed case names would bring the behavior of the `tracing_subscriber` crate back into alignment those other crates in this regard. ## Solution Accept mixed case names for log levels in directive strings. This PR includes two commits: 1. The first adds unit tests that demonstrate the mixed case logging level names being rejected. 2. The second introduces an adjustment to `DIRECTIVE_RE` that accepts mixed case logging level names. With this change, the tests again all pass. * [1 of 2]: subscriber: add more parse_directives* tests These test parse_directives() against strings that contain only a legit log level name. The variants that submit the mixed case forms are currently failing: $ cargo test --lib 'filter::env::directive::test::parse_directives_' ... failures: filter::env::directive::test::parse_directives_global_bare_warn_mixed filter::env::directive::test::parse_directives_ralith_mixed test result: FAILED. 12 passed; 2 failed; 0 ignored; 0 measured; 61 filtered out Fix to come in a follow-up commit. Co-authored-by: Eliza Weisman <[email protected]> Signed-off-by: Alan D. Salewski <[email protected]> * [2 of 2]: subscriber: directives: accept log levels in mixed case Fix issue demonstrated by unit tests in commit f607b7f. With this commit, the unit tests all pass. The 'DIRECTIVE_RE' regex now uses a case-insensitive, non-capturing subgroup when matching log level names in logging directive strings. This allows correctly capturing not only, e.g., "info" and "INFO" (both of which were also accepted previously), but also "Info" and other combinations of mixed case variations for the legit log level names. This change makes the behavior of tracing-subscriber more consistent with that of the `env_logger` crate, which also accepts mixed case variations of the log level names. Co-authored-by: Eliza Weisman <[email protected]> Signed-off-by: Alan D. Salewski <[email protected]>
Update the closing-spans link.
We should allow configuring whether or not to display module_path or file/line in output. Co-authored-by: 李小鹏 <[email protected]> Co-authored-by: Jane Lusby <[email protected]> Co-authored-by: Eliza Weisman <[email protected]>
## Motivation Tracing's examples depend on a number of external crates that the core `tracing` crates don't depend on. Sometimes, crates that we only depend on for examples will break compatibility with our MSRV. It's not ideal to bump our supported MSRV just for an example, since the actual `tracing` crates will still build fine. ## Solution Instead, this PR updates the CI config to exclude examples from the MSRV check. This way, we don't have to bump MSRV for examples-only dependencies. Signed-off-by: Eliza Weisman <[email protected]>
## Motivation Fixes the race condition outlined in #1120 . ## Solution `Worker` now uses a 2 stage shutdown approach. The first shutdown signal is sent through the main message channel to the `Worker` from `WorkerGuard` when it is dropped. Then `WorkerGuard` sends a second signal on a second channel that is zero-capacity. This means It will only succeed a `send()` when a `recv()` is called on the other end. This guarantees that the `Worker` has flushed all it's messages before the `WorkerGuard` can continue with its drop. With this solution I'm not able to reproduce the race anymore using the provided code sample from #1120 Co-authored-by: Zeki Sherif <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch backports a number of non-breaking changes from
mastertov0.1.x:These changes have already been approved on
master, so I'll just goahead and merge this once CI passes.
I'll rebase merge this so all the commits land individually.