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

Lower log message level for nailgun to reduce verbosity #20946

Merged
merged 11 commits into from
May 27, 2024

Conversation

alonsodomin
Copy link
Contributor

Nailgun starting log message makes Pants output very verbose and could actually feel somewhat annoying specially when dealing with repos that not only have a lot of JVM code, but also may rely on JVM tools to perform some of the tasks.

Beside, it feels like the log message's audience would be Pants' maintainers or plugin developers are not so targeted to provide special info about the build to end users.

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

I agree about it being noisy.. but if it's expensive, we could maybe keep a brief message to the fact as info! for visibility, while deferring the details to the debug log.

@alonsodomin
Copy link
Contributor Author

It is somewhat expensive but I truly wonder how much value this adds to actual end users. In the past years using Pants I found that the vast majority of devs don't really care about nailgun, it's a Pants implementation detail that they feel is leaking out. I understand the purpose of the message but must admit that, while I find it value when developing plugin or enhancement in Pants, I found it annoying when using Pants as a tool because like them, at that point I don't care much about a nailgun server being spawned as I trust Pants to do its thing correctly.

Also consider that we only log at info the creation of the instance but not when it's killed (the kill log message is at debug level), which can give to an end user the wrong feeling that new instances are being spawned all the time and never killed.

I compare this to, for example, creating a connection to a database via a connection pool. Creating a connection is expensive (not as much as starting nailgun but more than a basic computation) yet the connection pool will not emit an info log message for every new connection that is created but if we have an issue that could be related to that, it could be possible to debug using the debug log level.

I'm into simplifying the message though but one of the things that make this message noisy is its location, as it will show up for every single instance of nailgun that is started, and when working in a vast repo this can mean seeing this message many times, sometimes big blocks of text with a slightly similar message when several of them are spawn at the same time.

Because of that, what about having an info message when the nailgun pool is initialized? Something like Nailgun pool with N instances? That will show once and while it will not notify about the expensive spawn operation, at least will let us know that how many Nailgun processes are being used in the background.

Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

I fully agree!

@benjyw
Copy link
Contributor

benjyw commented May 22, 2024

I classified this as "documentation" - since it is user-noticeable, but not an API change. We've done this in the past with such changes.

@benjyw
Copy link
Contributor

benjyw commented May 22, 2024

@alonsodomin Note the CI errors, and in particular that you must now update the release notes.

@alonsodomin alonsodomin merged commit b761705 into pantsbuild:main May 27, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants