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

Add cancelLogging method #118

Closed
wants to merge 5 commits into from
Closed

Add cancelLogging method #118

wants to merge 5 commits into from

Conversation

DartAndrik
Copy link

@DartAndrik DartAndrik commented Aug 7, 2022

To avoid cases like:

  1. Somebody use logging package for plugin/package development.
  2. I used this 3rd party solution and logging package for my development.
  3. We do not use hierarchicalLoggingEnabled = true
  4. Logger in 3rd party plugin/package can be configured just on the fly.
  5. Somewhere in 3rd party plugin/package used packageLogger.clearListeners()

As the result i have my app logger deconfigured because of clearListeners method logic(stream controller for my logger was cleared too).

For examlpe package GoRouter uses setLogging method and i have to be carefull with the sequence of my app logger initialization and creation the instance of GoRouter. GoRouter uses GoRouter.setLogger first time just while constructing with clearListeners for GoRouter(debugLogDiagnostics: true). Even more, i have to check every time that GoRouter.setLogging have nor been used again latter by other developers in my app.

Version changed to 2.0.0 due to breaking changes. Class Logger is implemented in some packages (as in build_runner_core with BuildForInputLogger).

@google-cla
Copy link

google-cla bot commented Aug 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jakemac53 jakemac53 self-requested a review August 15, 2022 18:51
@jakemac53 jakemac53 removed their request for review August 29, 2022 19:11
@jakemac53
Copy link
Contributor

cc @devoncarew can you assign this to somebody in the ecosystem team to review? I meant to take a look but obviously haven't had a chance to, so unassigned myself.

@devoncarew
Copy link
Member

devoncarew commented Aug 30, 2022

@DartAndrik - thanks for the PR! I have a few questions to help get my head around the problem (in the meantime though, in order for us to accept contributions, you'll need to sign the CLA):

  • another client in the same app is calling what's essentially a global method - removing all listeners from the root logger
  • this means that you loose the listener you've set up for your app; messages are still passed to the logger but you no longer receive them
  • your proposal is to introduce a new method - cancelLogging - which that 3rd party package (go_router) should call in preference to clearListeners? So, this issue wouldn't really be resolved until v2 of logging was published and go_router was updated to call it?

Generally, we want to be very cautious about a major version rev. of a package like this - used by many other packages. We'll want to prefer a minor version rev. instead (this PR as written would be a minor version rev. - adding new API).

I'm curious why go_router is calling clearListeners in the first place - why it feels like it needs to clear all logging listeners. And why there's a need in general to terminate a logger like that (_controller?.close(); _controller = null;). It seems like a better pattern would be to just allow stream listeners of a logger to unsubscribe themselves.

@DartAndrik
Copy link
Author

@devoncarew Yes, all 3 points are correct. And i saw that it can be fixed in Go_Router. In different ways: from bool useLogging for example to any other log package i guess.
But as the Go_Router already moved to flutter/packages and its issues merged with flutter issues too, i think it can not use other solutions for logging (and why does it have to?, logging is a simple and great solution).

The main idea is to add cancelLogging to have the possibility to disable logs only for some scope (current logger and all down hierarchy child loggers) or only current logger. It is helpful for example for runtime flexible separated log management in app or from your app debug menu and doesn't destroy the whole logging system after disabling some feature loggers. 

And yes. You are right. The other idea is to replace clearListeners on cancelLogging in the Go_Router package(and use cancelLogging for other packages for such cases).

As for the major version  -  i totally agree. But i see that the Logger implementation is used in some packages.

@jakemac53
Copy link
Contributor

As for the major version  -  i totally agree. But i see that the Logger implementation is used in some packages.

Right, we do need to be careful here because we know the api is implemented. We could do a forward fix for package:build potentially... but it would leave out there some broken combinations of packages, and we also know that many users of build_runner are stuck on old versions because of being stuck on old analyzer versions. We could tell them to pin package:logging to an older version though if necessary.

@devoncarew
Copy link
Member

From talking to one of the go_router maintainers (@johnpryan), the use of clearListeners was unintentional - they were unaware that it would affect the root logger instead of just the one they were using for their package.

My guess is that they'll remove the call from that package. That should solve the immediate issue.

It still does leave open some questions about potential API design improvements to this package; things that would make unintentional interactions between package uses less likely.

@devoncarew
Copy link
Member

@DartAndrik - thanks for the contribution and for identifying the issue. Given the upcoming fix in flutter/packages#2533, I don't think we'll need to adjust the logging API in order to address the clearListeners() issue.

For posterity, here are some existing issues and discussions about package:logging API improvements:
#51 (comment), #43, #20 (comment), #60, ....

@devoncarew devoncarew closed this Aug 31, 2022
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.

None yet

3 participants