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

maven-plugin: Support increasing maven log level #4148

Merged
merged 2 commits into from Aug 21, 2023

Conversation

mensinda
Copy link
Contributor

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This PR adds a new property to the maven plugin, which allows increasing the log level with a new logLevel property.

Our primary use case / problem is that liquibase has a lot of INFO level logs. For instance, the Loading artifacts into URLClassLoader section is already bigger than the rest of the non-liquibase maven output for some of our projects. To solve this we want to only increase the log level of the liquibase plugin (so no mvn --quiet). Sadly, maven does not support changing the log level for a specific plugin inside the pom.xml. Thus, this PR is required.

Things to be aware of

  • Impact on the codebase: relatively minimal, since there is only one new class, and the logger is changed at a central location.

Things to worry about

None

Additional Context

I am aware that there already exists the maven SLF4J logging configuration. However this does not fit our need since this is a maven installation and not project configuration.

@nvoxland
Copy link
Contributor

nvoxland commented May 3, 2023

Initial/Pre-Review Thoughts

I'm a bit torn on what I think on this PR, and don't have an off-hand answer so I'll move it along the process for more discussion.

Like you said, Maven has their built-in functionality for managing the logs and my base theory is that Liquibase should be simply plugging into the existing logging system and not trying to fight it or change it. That if we try to go our own way with the logging, it ends up with strange edge cases due to conflicts in configurations. At the same time, I get that the maven logging config has limitations on how it can be configured and how well it's documented.

I also wonder if the root problem of Our primary use case / problem is that liquibase has a lot of INFO level logs is really best solved by looking at what is at info level and fixing that? The info level should be "what a regular user would expect as CLI output" aka "reasonably significant messages that will make sense to end users and system administrators" and a lot of that sounds like it fails that test.

So I'll open it up to the larger group:

  • Does fixing the log level of a few messages solve the problem best?
  • Do we want to do more than rely on what maven has configured for logging?
  • Is this implementation the right answer to the problems? How is it handling edge cases like "what if people are also configuring things like levels and channels in slf4j?" or "how does it work with simpler -X type log-related flags?" etc?

@mensinda
Copy link
Contributor Author

mensinda commented May 4, 2023

Does fixing the log level of a few messages solve the problem best?

This would partially solve my problem:

First of all, here would be my wishlist for logs that should just be debug logs for all scenarios:

  • The Loading artifacts into URLClassLoader logs here. Actually, all the info logs in this class could IMHO be downgraded to debug. My reasoning: If you are at the point to consider checking the classpath, you would have probably used mvn -X already.
  • The big liquibase ASCII art log also does not add anything (even the liquibase version is already logged by maven).
  • The Reading resource: logged here -- if you are wondering why your changeset did not execute use debug.

Additionally, the log level of Running Changeset: here, ChangeSet XXX ran successfully in here, and everything in between (Table FOO created, etc.) should be configurable. This is because with our setup, we generate the test database for our unittests with liquibase from scratch. We use the same liquibase scripts that we also use for our production DB. Thus, over the years we have accumulated a lot of changesets wich will be all executed each mvn clean verify run. This adds a lot of noice (the lines of liquibase loggs exceed the lines of test logs by a factor of at least 10).

Essentially, my goal is for our maven test DB workflow that liquibase only loggs a fixed number of lines, regardless of how big the classpath is and how many changesets are executed. However, changing the changeset log level for other use-cases (incremental DB updates) would certainly be a mistake.

@kevin-atx kevin-atx added TypeEnhancement RiskMedium Changes that require more testing or that affect many different code paths. and removed RiskMedium Changes that require more testing or that affect many different code paths. labels May 4, 2023
@kevin-atx kevin-atx added the RiskMedium Changes that require more testing or that affect many different code paths. label May 22, 2023
@filipelautert
Copy link
Collaborator

@StevenMassaro from MDC point of view, is this change ok?

@filipelautert filipelautert added sprint2023-52 SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Jun 5, 2023
@StevenMassaro
Copy link
Contributor

@StevenMassaro from MDC point of view, is this change ok?

I don't think this should have an impact. However, if someone were to decrease the log level, like to SEVERE, they may miss out on some MDC logging that they ordinarily would get with INFO/DEBUG/etc. However, that's intended functionality.

@filipelautert filipelautert self-assigned this Jun 5, 2023
Copy link
Collaborator

@filipelautert filipelautert left a comment

Choose a reason for hiding this comment

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

After all the reviews here this PR seems good to be moved along.

@rberezen
Copy link
Contributor

This pull request introduces a new property to the liquibase-maven-plugin, enabling the ability to change the log level using the 'logLevel' property.

  • Functional and test harness executions passing.
  • Tested manually.

APPROVED

@filipelautert filipelautert removed the request for review from XDelphiGrl June 28, 2023 12:47
@filipelautert filipelautert merged commit f7e0383 into liquibase:master Aug 21, 2023
32 of 40 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Aug 21, 2023
@mensinda mensinda deleted the mavenLogLevel branch August 28, 2023 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RiskMedium Changes that require more testing or that affect many different code paths. SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions sprint2023-52 TypeEnhancement
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants