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

subscriber: fix issue with EnvFilter::builder().parse #2052

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

Ma124
Copy link
Contributor

@Ma124 Ma124 commented Apr 6, 2022

Motivation

See issue #2046.
When using calling Builder::parse or Builder::parse_lossy with an empty
string an error is produced. This happens for example when EnvFilter::from_default_env()
is called, but the RUST_LOG variable is unset.
This regression was introduced by #2035.

Solution

Filter any empty directives. This allows the whole string to be empty, as well as leading
and trailing commas.
A unit test was added for Builder::parse, but not Builder::parse_lossy because it (per definition)
doesn't produce any side effects visible from tests when erroring.

@Ma124 Ma124 requested review from hawkw, davidbarsky and a team as code owners April 6, 2022 21:00
@Ma124
Copy link
Contributor Author

Ma124 commented Apr 6, 2022

I'm pretty sure this works correctly, but I had trouble testing it with my own applications because of reasons unrelated to these changes.

Maybe, you as maintainers, encountered this when testing your changes:

tracing-subscriber = { version = "0.3.10", features = ["env-filter"] }
tracing-subscriber = { git = "https://github.com/tokio-rs/tracing.git", tag = "tracing-subscriber-0.3.10", features = ["env-filter"] }

When I have the first line in my Cargo.toml and use error!, the message is correctly printed.
When I have the second line (instead of the first), which should pull exactly the same version, in my Cargo.toml, suddenly no messages are printed. This happens with RUST_LOG=trace as well.

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.

Thanks, this looks good to me!

@hawkw hawkw enabled auto-merge (squash) April 8, 2022 00:19
@hawkw
Copy link
Member

hawkw commented Apr 8, 2022

I'm pretty sure this works correctly, but I had trouble testing it with my own applications because of reasons unrelated to these changes.

Maybe, you as maintainers, encountered this when testing your changes:

tracing-subscriber = { version = "0.3.10", features = ["env-filter"] }
tracing-subscriber = { git = "https://github.com/tokio-rs/tracing.git", tag = "tracing-subscriber-0.3.10", features = ["env-filter"] }

When I have the first line in my Cargo.toml and use error!, the message is correctly printed. When I have the second line (instead of the first), which should pull exactly the same version, in my Cargo.toml, suddenly no messages are printed. This happens with RUST_LOG=trace as well.

Because the master branch contains in-development changes for the upcoming tracing 0.2 release, it isn't compatible with the currently released versions. To test changes from the master branch locally, it will be necessary to patch all tracing crates to use the Git versions, not just the one that changed. Hopefully that's helpful in the future!

@hawkw hawkw merged commit c7407c4 into tokio-rs:master Apr 8, 2022
hawkw pushed a commit that referenced this pull request Apr 8, 2022
…#2052)

## Motivation

See issue #2046. When using calling [`Builder::parse`] or
[`Builder::parse_lossy`] with  an empty string an error is produced.
This happens for example when `EnvFilter::from_default_env()` is called,
but the `RUST_LOG` variable is unset. This regression was introduced by
#2035.

## Solution

Filter any empty directives. This allows the whole string to be empty,
as well as leading and trailing commas. A unit test was added for
[`Builder::parse`], but not [`Builder::parse_lossy`] because it (per
definition) doesn't produce any side effects visible from tests when
erroring.

Fixes #2046

[`Builder::parse`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L151=
[`Builder::parse_lossy`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L135=
@hawkw hawkw mentioned this pull request Apr 8, 2022
hawkw pushed a commit that referenced this pull request Apr 9, 2022
…#2052)

## Motivation

See issue #2046. When using calling [`Builder::parse`] or
[`Builder::parse_lossy`] with  an empty string an error is produced.
This happens for example when `EnvFilter::from_default_env()` is called,
but the `RUST_LOG` variable is unset. This regression was introduced by
#2035.

## Solution

Filter any empty directives. This allows the whole string to be empty,
as well as leading and trailing commas. A unit test was added for
[`Builder::parse`], but not [`Builder::parse_lossy`] because it (per
definition) doesn't produce any side effects visible from tests when
erroring.

Fixes #2046

[`Builder::parse`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L151=
[`Builder::parse_lossy`]: https://github.com/tokio-rs/tracing/blob/cade7e311848227348c9b3062e4a33db827a0390/tracing-subscriber/src/filter/env/builder.rs#L135=
hawkw added a commit that referenced this pull request Apr 9, 2022
# 0.3.11 (Apr 9, 2022)

This is a bugfix release for the `Filter` implementation for `EnvFilter` added
in [v0.3.10].

### Fixed

- **env-filter**: Added missing `Filter::on_record` callback to `EnvFilter`'s
  `Filter` impl ([#2058])
- **env-filter**: Fixed method resolution issues when calling `EnvFilter`
  methods with both the `Filter` and `Layer` traits in scope ([#2057])
- **env-filter**: Fixed `EnvFilter::builder().parse()` and other parsing methods
  returning an error when parsing an empty string ([#2052])

Thanks to new contributor @Ma124 for contributing to this release!

[v0.3.10]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.10
[#2058]: #2058
[#2057]: #2057
[#2052]: #2052
hawkw added a commit that referenced this pull request Apr 9, 2022
# 0.3.11 (Apr 9, 2022)

This is a bugfix release for the `Filter` implementation for `EnvFilter` added
in [v0.3.10].

### Fixed

- **env-filter**: Added missing `Filter::on_record` callback to `EnvFilter`'s
  `Filter` impl ([#2058])
- **env-filter**: Fixed method resolution issues when calling `EnvFilter`
  methods with both the `Filter` and `Layer` traits in scope ([#2057])
- **env-filter**: Fixed `EnvFilter::builder().parse()` and other parsing methods
  returning an error when parsing an empty string ([#2052])

Thanks to new contributor @Ma124 for contributing to this release!

[v0.3.10]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.10
[#2058]: #2058
[#2057]: #2057
[#2052]: #2052
kaffarell pushed a commit to kaffarell/tracing that referenced this pull request May 22, 2024
# 0.3.11 (Apr 9, 2022)

This is a bugfix release for the `Filter` implementation for `EnvFilter` added
in [v0.3.10].

### Fixed

- **env-filter**: Added missing `Filter::on_record` callback to `EnvFilter`'s
  `Filter` impl ([tokio-rs#2058])
- **env-filter**: Fixed method resolution issues when calling `EnvFilter`
  methods with both the `Filter` and `Layer` traits in scope ([tokio-rs#2057])
- **env-filter**: Fixed `EnvFilter::builder().parse()` and other parsing methods
  returning an error when parsing an empty string ([tokio-rs#2052])

Thanks to new contributor @Ma124 for contributing to this release!

[v0.3.10]: https://github.com/tokio-rs/tracing/releases/tag/tracing-subscriber-0.3.10
[tokio-rs#2058]: tokio-rs#2058
[tokio-rs#2057]: tokio-rs#2057
[tokio-rs#2052]: tokio-rs#2052
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.

2 participants