Targets::with_targets ignores default

[recmo]

Bug Report

Version

`cargo tree | grep tracing`

│   │   ├── tracing-core v0.1.21
│   │   └── tracing-error v0.1.2
│   │       ├── tracing v0.1.29
│   │       │   ├── tracing-attributes v0.1.18 (proc-macro)
│   │       │   └── tracing-core v0.1.21 (*)
│   │       └── tracing-subscriber v0.2.25
│   │           └── tracing-core v0.1.21 (*)
│   └── tracing-error v0.1.2 (*)
│   │   │   │   │   └── tracing v0.1.29 (*)
│   │   │   │   └── tracing v0.1.29 (*)
│   │   │   │   ├── tracing v0.1.29 (*)
│   │   │   │   └── tracing v0.1.29 (*)
│   │   │   ├── tracing v0.1.29 (*)
│   │   │   └── tracing-futures v0.2.5
│   │   │       └── tracing v0.1.29 (*)
│   │   └── tracing-core v0.1.21 (*)
│   ├── tracing v0.1.29 (*)
│   ├── tracing-core v0.1.21 (*)
│   └── tracing-subscriber v0.3.3
│       ├── tracing v0.1.29 (*)
│       ├── tracing-core v0.1.21 (*)
│       ├── tracing-log v0.1.2
│       │   └── tracing-core v0.1.21 (*)
│       └── tracing-serde v0.1.2
│           └── tracing-core v0.1.21 (*)
│   │   │   │   │   │   ├── tracing v0.1.29
│   │   │   │   │   │   │   └── tracing-core v0.1.21
│   │   │   ├── tracing v0.1.29 (*)
│   │   │   ├── tracing-futures v0.2.5 (*)
│   │   ├── tracing v0.1.29 (*)
│   │   ├── tracing-futures v0.2.5 (*)
│       ├── tracing v0.1.29 (*)
│       ├── tracing v0.1.29 (*)
│       └── tracing-futures v0.2.5 (*)
├── tracing v0.1.29 (*)
├── tracing-futures v0.2.5 (*)
├── tracing-opentelemetry v0.16.0
│   ├── tracing v0.1.29 (*)
│   ├── tracing-core v0.1.21 (*)
│   ├── tracing-log v0.1.2 (*)
│   └── tracing-subscriber v0.3.3 (*)
├── tracing-subscriber v0.3.3 (*)
├── tracing-test v0.2.1
│   ├── tracing-core v0.1.21 (*)
│   ├── tracing-subscriber v0.3.3 (*)
│   └── tracing-test-macro v0.2.1 (proc-macro)

Platform

Darwin MacBook-Pro-16-inch-2021-2.local 21.2.0 Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:41 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T6000 arm64

Crates

tracing_subscriber

Description

Merging two Targets objects using first.with_targets(second) ignores the default value set in second.

Example

let first = Targets::new()
    .with_default(Level::INFO)
    .with_target("app", Level::DEBUG);
let second = Targets::new()
    .with_default(Level::DEBUG)
    .with_target("app", Level::TRACE);
let combined = first.with_targets(second);
dbg!(&combined);

results in

[src/cli/logging.rs:56] &combined = Targets(
    DirectiveSet {
        directives: [
            StaticDirective {
                target: Some(
                    "app",
                ),
                field_names: [],
                level: LevelFilter::TRACE,
            },
            StaticDirective {
                target: None,
                field_names: [],
                level: LevelFilter::INFO,
            },
        ],
        max_level: LevelFilter::TRACE,
    },
)

where the app directive is correctly overridden by second, but the original default directive is retained.

In fact, there appears to be no method to access the default level on a Targets, so I can not even write a workaround.

I'm using this pattern to combine the --verbose CLI argument (first) with a --log-filter argument (second). I want the log-filter to take precedence for all directives, including default.

[benesch]

I just ran into this too! I'd be happy to prepare a PR if someone has an opinion on what the appropriate fix is.

[hawkw]

What do you think the expected behavior for the default directive when merging Targets should be? Currently, it does not change the default because I thought that changing the default would be more surprising than not changing it. If most people would expect the default directive to also be changed, we could certainly change the behavior here, but I'm not sure whether that would actually be less surprising.

Another potential option is to always pick the more verbose default, rather than always overwriting the default from the first set of targets with the default from the second one.

Alternatively, we could add an accessor that returns the default level, so that the user can use that to explicitly implement whatever merging logic they want, and document in the with_targets method that this method doesn't effect the default directive. I think that might be the best approach, because it allows users of the API to choose how the default directives are merged.

How do others feel about that solution?

[benesch]

FWIW, the behavior I expected is that the last target to specify a "thing" wins, whether that "thing" is a specific target or the default. Isn't that the way all the other merging works? (If not then I am misinformed.)

I think that might be the best approach, because it allows users of the API to choose how the default directives are merged.

This works for me, though!