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

Re-design Logger hierarchy #3603

Closed
lihaoyi opened this issue Sep 26, 2024 · 7 comments
Closed

Re-design Logger hierarchy #3603

lihaoyi opened this issue Sep 26, 2024 · 7 comments
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Sep 26, 2024

The Logger interface is a bit of a mess, doing a bunch of different things some Mill-internal and some user-facing. #3577 extends it a bit but makes it even more messy.

Probably we should have more narrow specialized Logger interfaces for different parts of the codebase, e.g. the logger inside a Task would have a ticker, whereas the logger outside would not, and a top-level logger would have more power to do things like configuring the prompt that task-specific loggers would not have. close() probably needs to be re-thought since the lifecycle of the various loggers is all different, with some short-lived and some long-live

The whole color scheme and debug/info/warning/error conventions also need to be redone. Currently infoColor is blue and normally used for "less important" things, but then logger.info uses it for normal-important things. And how should a user decide between info and just println? When should subclasses of Logger put logic into overriding info vs injecting logic into stdout/stderr streams?

Arguably, all the concrete FooLogger classes should be private[mill], since there's no reason that a Mill user would need to instantiate their own PrintLogger or whatever.

This would break bincompat and would have to go into 0.13.0

@lihaoyi lihaoyi added this to the 0.13.0 milestone Sep 26, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 26, 2024

At time of writing this is the current logging hierarchy that is typically available inside any Mill task:

MultiLogger(
    PrefixLogger(
        PrefixLogger(
            PromptLogger("foo.test"),
            "",
            ""
        ),
        "[106] ",
        ""
    ),
    FileLogger(/Users/lihaoyi/Github/mill/example/scalalib/testing/4-test-sharding/out/foo/test/test.log)
)

@lihaoyi lihaoyi mentioned this issue Mar 5, 2025
lihaoyi added a commit that referenced this issue Mar 5, 2025
A step towards #3603. For now
just doing relatively shallow cleanups, but hopefully that paves the way
to bigger refactorings


The main external-facing change is renaming
`inStream`/`outputStream`/`errorStream` to `streams.in`/`out`/`err`,
though the internal refactorings are more substantial
@lihaoyi
Copy link
Member Author

lihaoyi commented Mar 6, 2025

With #4653 and #4658, most of the superficial cleanups have been done. The next big questions are around the architecture of the logging API

What do we want to do with debug, info, and error?

  1. Do we want to immediately fold them into stderr? That would simplify things considerably, but lose flexibility: e.g. we would no longer be able to separate these logs into separate files if we so desired

  2. Do we want to keep them separate? That is what we do now, and it adds considerable complexity, which we currently don't really make use of or benefit from

Different logging libraries have varying degrees of sophistication in how they handle log levels. Currently Mill just stuffs them all into stderr with different colors, and debug is silenced unless the --debug flag is passed. If that's all we want to do, we can simplify the code considerably by hardcoding that assumption in the code. If we want to do more than that, we should figure out what exactly we want to do and design the API appropriately. Currently we have the worst of both worlds with all the complexity but no benefits

How should --ticker false interact with --disable-prompt?

Currently the various combinations of --ticker {true,false} and --disable-prompt` result in 5 different configurations for the CLI.

  1. --ticker false disables the multi-line prompt with no replacement, and removes some (but not all) of the line-prefixes
  2. --disable-prompt swaps from the new multi-line prompt introduced in 0.11.x to the older single-line ticker.
  3. If neither flag is passed, the prompt runs in "non-interactive" mode where instead of using terminal control codes to live update your terminal, it instead prints out a snapshot periodically to stdout (default every 60s)

Currently, only three scenarios are actively used and tested

  1. Currently basically all manual testing is done with the default setup, while all integration and example testing is done with --disable-ticker.
  2. The non-interactive mode is used in CI
  3. --disable-prompt doesn't seem to be exercised anywhere at all, though manually testing it it seems to still work (kind of, the ticker sometimes disappears?)

Do we need all 5 options, or would fewer options be sufficient?

lihaoyi added a commit that referenced this issue Mar 6, 2025
Another part of #3603

More narrowing of the `Logger` interface so we can figure out what
really needs to be there

- Moved `colored`, `infoColor`, `errorColor` to the `prompt` object
since it's not typically overriden
- Inlined the `subLogger` calls at its one call-site
lihaoyi added a commit that referenced this issue Mar 6, 2025
Part of #3603

We've been using the new multi-line `PromptLogger` for a while since
0.12.0 was released back in Aug 2024, and it's worked great. So we can
remove the old implementation for 0.13.0 due in mid 2025, and deprecate
and hide the `--remove-prompt` flag that was introduced to flip back to
the old implementation.

It turns out that we were still using the old `PrintLogger` in a bunch
of places - unit tests, testrunner, etc. - and I had to replace those
use sites either with `PromptLogger` or with just `println`

Also made some adjustments to `enableTicker` handling to keep it
internal within `Logger` so it is more consistently applied, and fixed
the logging of skipped tasks in `--keep-going`
lihaoyi added a commit that referenced this issue Mar 6, 2025
Part of #3603

Mostly just describing what they do now
@lihaoyi
Copy link
Member Author

lihaoyi commented Mar 6, 2025

Going to call this done. The design can always be better, and we can continue improving it, but I think this meets the minimum threshold of quality and tidyness for now

@lihaoyi lihaoyi closed this as completed Mar 6, 2025
@lolgab
Copy link
Member

lolgab commented Mar 6, 2025

I would love to be able to set a global level as I can do with sbt.
Also, we are missing warn, which many tools support (the scala compiler as well).

@lefou
Copy link
Member

lefou commented Mar 6, 2025

I would love to be able to set a global level as I can do with sbt.

I'm not a sbt user. Can you explain, how that would look like for Mill?

Also, we are missing warn, which many tools support (the scala compiler as well).

Yeah, I'd love to have that, too.

@lihaoyi
Copy link
Member Author

lihaoyi commented Mar 6, 2025

@lolgab @lefou you guys may have to drive the improvements here, I'm not really a logging system connoisseur and don't really have opinions what makes a logging system good or not, but if you guys do then 0.13.0 is our chance to improve things

@lolgab
Copy link
Member

lolgab commented Mar 6, 2025

@lihaoyi I can start by opening a PR to add def warn to Logger to be able to print yellow warnings.

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

No branches or pull requests

3 participants