subscriber: directives: accept legit log level names in mixed case (env_logger compat)

[salewski]

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::Directives 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.

[hawkw]

Thanks for the PR! I definitely agree that if env_logger accepts mixed-case level names, then we ought to accept them as well.

I think the CI failure on Rust 1.42.0 is due to an upstream dependency breaking compatibility with our MSRV — that's not related to this PR at all. When that's fixed on master, you can update this branch, and the CI run should pass.

[hawkw]

looks good to me once we get a green CI build — I left a couple suggestions but they're not important, so feel free to take it or leave it!

[hawkw]

Since this PR adds so many new test cases that are all pretty equivalent, I wonder if it's worth trying to factor out some of the repeated code. For example, we could do something like:

Suggested change

	#[test]
	fn parse_directives_global_bare_warn_lc() {
	// test parse_directives with no crate, in isolation, all lowercase
	let dirs = parse_directives("warn");
	assert_eq!(dirs.len(), 1, "\nparsed: {:#?}", dirs);
	assert_eq!(dirs[0].target, None);
	assert_eq!(dirs[0].level, LevelFilter::WARN);
	assert_eq!(dirs[0].in_span, None);
	}
	#[test]
	fn parse_directives_global_bare_warn_uc() {
	// test parse_directives with no crate, in isolation, all uppercase
	let dirs = parse_directives("WARN");
	assert_eq!(dirs[0].target, None);
	assert_eq!(dirs[0].level, LevelFilter::WARN);
	assert_eq!(dirs[0].in_span, None);
	}
	#[test]
	fn parse_directives_global_bare_warn_mixed() {
	// test parse_directives with no crate, in isolation, mixed case
	let dirs = parse_directives("wArN");
	assert_eq!(dirs[0].target, None);
	assert_eq!(dirs[0].level, LevelFilter::WARN);
	assert_eq!(dirs[0].in_span, None);
	}
	fn test_parse_bare_level(directive: &str, level: LevelFilter) {
	let dirs = parse_directives(directive);
	assert_eq!(dirs.len(), 1, "\nparsed: {:#?}", dirs);
	assert_eq!(dirs[0].target, None);
	assert_eq!(dirs[0].level, LevelFilter::WARN);
	assert_eq!(dirs[0].in_span, None);
	}
	#[test]
	fn parse_directives_global_bare_warn_lc() {
	// test parse_directives with no crate, in isolation, all lowercase
	test_parse_bare_level("warn", LevelFilter::WARN);
	}
	#[test]
	fn parse_directives_global_bare_warn_uc() {
	// test parse_directives with no crate, in isolation, all uppercase
	test_parse_bare_level("WARN", LevelFilter::WARN);
	}
	#[test]
	fn parse_directives_global_bare_warn_mixed() {
	// test parse_directives with no crate, in isolation, mixed case
	test_parse_bare_level("wArN", LevelFilter::WARN);
	}

If you don't think this is worth the effort, I'm happy to merge this PR either way — just a thought!

[salewski]

Since this PR adds so many new test cases that are all pretty equivalent, I wonder if it's worth trying to factor out some of the repeated code. For example, we could do something like:

If you don't think this is worth the effort, I'm happy to merge this PR either way — just a thought!

Thanks for the suggestion; that's clearly better. I'll incorporate it and rebase.

[salewski]

I incorporated this idea into my push from earlier tonight.

[hawkw]

If you don't mind rebasing the current master, #1128 should have fixed the CI failure. Thanks!

[salewski]

If you don't mind rebasing the current master, #1128 should have fixed the CI failure. Thanks!

I just pushed a rebase, but the rustfmt step in the "style" test isn't happy. Let me fix that up, too, and I'll push another rebase shortly. More soon...

[salewski]

If you don't mind rebasing the current master, #1128 should have fixed the CI failure. Thanks!

I just pushed a rebase, but the rustfmt step in the "style" test isn't happy. Let me fix that up, too, and I'll push another rebase shortly. More soon...

I just pushed those changes.

[salewski]

If you don't mind rebasing the current master, #1128 should have fixed the CI failure. Thanks!

I just pushed a rebase, but the rustfmt step in the "style" test isn't happy. Let me fix that up, too, and I'll push another rebase shortly. More soon...

I just pushed those changes.

Those changes were not greeted with unanimous green checkmark applause by the CI checks :-)

(Other CI stuff unrelated to this PR)

[salewski]

These changes were rebased on top of the updates to master from issue #1131. The CI checks are all green now.

[hawkw]

Okay, this looks great, thank you!