Make detached Loggers work regardless of hierarchicalLoggingEnabled

[jamesderlin]

Previously detached Loggers could log messages only if
hierarchicalLoggingEnabled was true. This makes no sense to me since
detached Loggers aren't part of a Logger hierarchy.

Fixes dart-lang/core#434.

[natebosch]

@jakemac53 - I'd like a second pair of eyes on this.

This could potentially resolve some of the weirdness we had run in to with this global variable.

[natebosch]

This LGTM. I'd like @jakemac53 to have a chance to look through it before we merge.

[jakemac53]

This looks good other than it is breaking (or seems that way, correct me if I am wrong).

We would need to talk about the ramifications of that, as it will require a complicated rollout.

[jakemac53]

If we do go forward and consider it a breaking change I would like to take the opportunity to go to 1.0 and we would also want to consider other breaking changes.

[natebosch]

This looks good other than it is breaking (or seems that way, correct me if I am wrong).

I don't think it is. Unless someone was specifically relying on a logger not working without the appropriate global variable value.

Is there a particular usage pattern that you think might be broken here? I also haven't found any broken tests to attribute to this change.

[jakemac53]

Previously if heirarchical logging was disabled, everything just went through the root logger no matter what (afaict).

Now, those logs would go only through the detached loggers regardless of that setting.

This seems like a good change, don't get me wrong, but it also seems breaking? Maybe its not breaking in a way that matters though.

[jamesderlin]

Previously if heirarchical logging was disabled, everything just went through the root logger no matter what (afaict).

Yes. But strictly speaking, any bug fix could be considered a breaking change since it'd result in a change in behavior. I consider the old behavior to be incorrect (Logger.detached does not behave intuitively based on the documentation).

I don't expect that this would break in a way that matters; if someone wanted to go through the root logger without hierarchicalLoggingEnabled, there'd be no point to using Logger.detached over the normal Logger constructor in the first place.

If we do go forward and consider it a breaking change I would like to take the opportunity to go to 1.0 and we would also want to consider other breaking changes.

If we're following semantic versioning, then breaking changes in 0.y.z releases are allowed in principle. (But breaking things haphazardly still would not be good for users, of course.)

[jakemac53]

Yes. But strictly speaking, any bug fix could be considered a breaking change since it'd result in a change in behavior. I consider the old behavior to be incorrect (Logger.detached does not behave intuitively based on the documentation).

There is a big difference between a bug that causes a crash or unusable result and one which actually results in potentially useful behavior but isn't to spec. The latter is a lot more breaking to fix (and is the category this falls into).

I don't expect that this would break in a way that matters; if someone wanted to go through the root logger without hierarchicalLoggingEnabled, there'd be no point to using Logger.detached over the normal Logger constructor in the first place.

I tend to agree with you that we can probably not call this breaking. The thing that does scare me a bit is the global nature of the Logging package. Anybody can create a logger (maybe detached, because they didn't understand what it meant, and it seemed "easier"). Then at the top level you usually have a single thing actually listening to logs and doing something with them. Obviously detached loggers were the exception, but they sort of worked that way previously (at least without hierarchical logging enabled).

One thing that gives me some hope is I think pretty much everybody always enables hierarchical logging.

If we're following semantic versioning, then breaking changes in 0.y.z releases are allowed in principle. (But breaking things haphazardly still would not be good for users, of course.)

Not with pubs version of semantic versioning - (see here):

Although semantic versioning doesn’t promise any compatibility between versions prior to
1.0.0, the Dart community convention is to treat those versions semantically as well. The 
interpretation of each number is just shifted down one slot: going from 0.1.2 to 0.2.0
indicates a breaking change, going to 0.1.3 indicates a new feature, and going to 0.1.2+1
indicates a change that doesn’t affect the public API.

[jakemac53]

Fwiw, there are 278 packages on pub that depend on this package.

So some caution here is advised. It is entirely possible that its much more "breaking" to actually do a breaking change than it is to make this change though.

[jamesderlin]

Is there an easy way to identify which of those 278 packages use Logger.detached?

[jakemac53]

Is there an easy way to identify which of those 278 packages use Logger.detached?

Not exactly easy, but there is the pub_crawl package which will fetch all packages from pub with a certain dependency for you into a local cache. It also support writing some custom validators which you could use to find all these calls, or you could just grep the cache dir that it creates. There is also the surveyor package which works off of these cache dirs and has some nicer utilites for deep analysis.

Using these probably involves a couple hours of work but is well worth the investment just to get an idea of how many people might be using Logger.detached so we can have a better idea of how breaking the change could potentially be.

I am perfectly fine with accepting this as a non-breaking release if we are reasonably confident the existing usage is minimal and/or not relying on the existing non-hierarchical logger behavior.

[jamesderlin]

I had to make some changes to pub_crawl to fetch packages with a specified dependency. (pub reports 278 packages that depend on package:logging, but pub_crawl fetched 333; not sure yet what's up with that. It also probably would have been faster to scrape the search results from pub.dev. =/ )

Anyway, after running pub_crawl and grep-ing through the resulting cache/ directory, I found 9 uses of Logger.detached in 6 packages. Of those, 6 sites set hierarchicalLoggingEnabled = true. The remaining 3 sites are from 2 packages: nyxx and s3_cache_image. Both of them use Logger.detached in library code to be consumed by other applications. Both also have code that directly sets Logger.root.level and Logger.root.onRecord, which seems like a bad practice. Neither sets .level or .onRecord on the detached Logger itself.

Therefore, this change would break logging for those packages in dependent applications that do not set hierarchicalLoggingEnabled = true. However, this change also would allow those packages to do what I think the authors intended, which is to have an independent Logger instance for each package.

If we think that's bad enough, I'm okay with bumping the version number to, say, 0.12.0-dev instead.

[jakemac53]

Thanks for doing the investigation here, I know its a pain but it comes with the territory :).

I had to make some changes to pub_crawl to fetch packages with a specified dependency. (pub reports 278 packages that depend on package:logging, but pub_crawl fetched 333; not sure yet what's up with that. It also probably would have been faster to scrape the search results from pub.dev. =/ )

The difference is likely that pub filters some results - it has some hidden abilities to mark packages as deprecated effectively and then it won't surface them. It also won't surface dev releases by default. The api probably doesn't do that tho so pub crawl finds them all.

Anyway, after running pub_crawl and grep-ing through the resulting cache/ directory, I found 9 uses of Logger.detached in 6 packages. Of those, 6 sites set hierarchicalLoggingEnabled = true. The remaining 3 sites are from 2 packages: nyxx and s3_cache_image.

Great, I looked at those packages briefly and I think it would be ok for us to break them. They aren't super popular packages, and one of them is currently broken anyways in the latest sdk. It might be nice to file issues on their github repos alerting them to the potential breakage though.

LGTM