Skip to content

Prefix logging for BSP #5154

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Collaborator

@alexarchambault alexarchambault commented May 19, 2025

When Mill runs as a BSP server, this makes it log in a similar way as when running Mill from the command-line, with […] prefixes.

For example, the small BSP session run in the test added in this PR logs the following to stderr:

[bsp] Trying to load BSP server...
[bsp] BSP server started
[1-buildInitialize] Entered buildInitialize
[1-buildInitialize] buildInitialize took 2 msec
[2-workspaceBuildTargets] Entered workspaceBuildTargets
[bsp-init-build.mill-61] [info] compiling 3 Scala sources to /Users/alexandre/projects/mill/mill-class-loading/out/integration/ide/bsp-server/local/daemon/testForked.dest/sandbox/run-1/out/mill-build/compile.dest/classes ...
[bsp-init-build.mill-61] [warn] package scala contains object and package with same name: caps.
[bsp-init-build.mill-61] [warn] This indicates that there are several versions of the Scala standard library on the classpath.
[bsp-init-build.mill-61] [warn] The build should be reconfigured so that only one version of the standard library is on the classpath.
[bsp-init-build.mill-61] [warn] one warning found
[bsp-init-build.mill-61] [info] done compiling
[bsp-init] SNAPSHOT
[2-workspaceBuildTargets] Evaluating 5 tasks
[2-workspaceBuildTargets] Done
[2-workspaceBuildTargets] Evaluating 1 task
[2-workspaceBuildTargets] Done
[2-workspaceBuildTargets] workspaceBuildTargets took 4197 msec
[3-loggingTest] Entered loggingTest
[3-loggingTest] Evaluating 1 task
[3-loggingTest-1] bspLoggingTest from System.out
[3-loggingTest-1] bspLoggingTest from System.err
[3-loggingTest-1] bspLoggingTest from Console.out
[3-loggingTest-1] bspLoggingTest from Console.err
[3-loggingTest-1] bspLoggingTest from Task.log.info
[3-loggingTest] Done
[3-loggingTest] loggingTest took 11 msec
[4-buildShutdown] Entered buildShutdown
[5-onBuildExit] Entered onBuildExit
Reload finished, result: mill.api.internal.BspServerResult$Shutdown$@6726cc69
Shutting down executor
[bsp] BSP session returned with mill.api.internal.BspServerResult$Shutdown$@6726cc69
Stopping server via handle...
[bsp] Exiting BSP runner loop. Stopping BSP server. Last result: Success(mill.api.internal.BspServerResult$Shutdown$@6726cc69)

This helps make sense of what's going on when working on BSP-releated features, but could also help Mill BSP users in case anything goes wrong.

@alexarchambault
Copy link
Collaborator Author

Includes #5153 for now

@alexarchambault

This comment was marked as resolved.

@alexarchambault
Copy link
Collaborator Author

alexarchambault commented May 19, 2025

About 1113602, without it, it seems System.{out,err} don't get captured in tasks run by the BSP server. In the example above, bspLoggingTest from System.out and bspLoggingTest from System.err get no prefix in that case. I wasn't able to reproduce that issue outside of BSP. That's what motivated #5155 initially.

final def info(s: String): Unit =
info(logKey, s)

def info(logKey: Seq[String], s: String): Unit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these changes to the Logger interface?

Logger is shared throughout Mill as part of core.api, and thus it really needs to be as simple as possible. Rather than adding a more verbose version of info/debug/etc., could we instead instantiate sub-classes of Logger inside MillBuildServer that have the correct logKey set?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we even use the PrefixLogger class for this? Adding a prefix to log lines is exactly what that class is meant to do

handlerTasks(
targetIds = _ => targetIds,
tasks = { case m: RunModuleApi => m.bspJvmRunTestEnvironment }
tasks = { case m: RunModuleApi => m.bspJvmRunTestEnvironment },
requestDescription = "jvmRunTestEnv",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need custom names for these? We already take the sourcecode.Name of each endpoint method, which I would have thought would give us enough information about what endpoint each log message is comming from

@@ -12,22 +12,44 @@ public DoubleLock(Lock lock1, Lock lock2) throws Exception {

@Override
public Locked lock() throws Exception {
return new DoubleLocked(lock1.lock(), lock2.lock());
Locked l1 = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this from this PR since it's unrelated

: CompletableFuture[InitializeBuildResult] =
handlerRaw(checkInitialized = false) {
: CompletableFuture[InitializeBuildResult] = {
val logger = createLogger()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to call createLogger() and pass it in to every handlerRaw? Could we instead make handlerRaw take the sourcecode.Enclosing and create the logger internally?

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.

2 participants