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

Expose logging levels configuration #600

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Oct 19, 2021

This is just an example how to expose logger levels (by logger name)
to configure their levels.

This is just an example how to expose logger levels (by logger name)
to configure their levels.
@cstamas cstamas self-assigned this Oct 19, 2021
@cstamas cstamas mentioned this pull request Oct 19, 2021
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

I pretty much like this idea, but I think it requires more thought and I will explain why: During Resolver work on sync context I have enabled TRACE of specific loggers only to get my result:

for buildId in $(eval echo {1..$BUILD_COUNT}); do \
  $MAVEN_HOME/bin/mvn clean compile -T1C -B -DskipTests -Dmaven.repo.local=$LOCAL_REPO \
  -Dorg.slf4j.simpleLogger.showThreadName=true -Dorg.slf4j.simpleLogger.showDateTime=true \
  -Dorg.slf4j.simpleLogger.log.org.eclipse.aether=trace -l $LOCK_FACTORY-$NAME_MAPPER-$buildId.log \
  -Daether.connector.basic.threads=8 -Daether.metadataResolver.threads=8 \
  -Daether.syncContext.named.time=60 \
  -Daether.syncContext.named.factory=$LOCK_FACTORY -Daether.syncContext.named.nameMapper=$NAME_MAPPER; \

Would it make sense not to introduce -XL, but rather have a generic -L/--logger with the mandatory arg of {loggerName}:{level}/{loggerName}={level}?

@@ -158,6 +160,7 @@ public CLIManager()
options.addOption( Option.builder( BUILDER ).longOpt( "builder" ).hasArg().desc( "The id of the build strategy to use" ).build() );
options.addOption( Option.builder( NO_TRANSFER_PROGRESS ).longOpt( "no-transfer-progress" ).desc( "Do not display transfer progress when downloading or uploading" ).build() );
options.addOption( Option.builder().longOpt( COLOR ).hasArg().optionalArg( true ).desc( "Defines the color mode of the output. Supported are 'auto', 'always', 'never'." ).build() );
options.addOption( Option.builder( VERBOSE_LOGGERS ).longOpt( "verboseLoggers" ).hasArg().desc( "Produce verbose output for loggers (comma separated list of logger names)" ).build() );
Copy link
Member

Choose a reason for hiding this comment

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

This sould be kebab case like other long options.

Copy link
Member

Choose a reason for hiding this comment

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

The desc should be similar to one of ACTIVATE_PROFILES, structurally.

slf4jConfiguration.setLoggerLevel( verboseLogger, Slf4jConfiguration.Level.DEBUG );
}
}

if ( cliRequest.verbose )
{
cliRequest.request.setLoggingLevel( MavenExecutionRequest.LOGGING_LEVEL_DEBUG );
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why any chance what this actually does? Since we cannot set it for the above?!

slf4jConfiguration.setLoggerLevel( verboseLogger, Slf4jConfiguration.Level.DEBUG );
}
}

if ( cliRequest.verbose )
Copy link
Member

Choose a reason for hiding this comment

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

Should -X and -XL be mutually exclusive?

@@ -48,7 +54,7 @@ public void setRootLoggerLevel( Level level )
value = "error";
break;
}
System.setProperty( "maven.logging.root.level", value );
System.setProperty( "maven.logging." + loggerName + ".level", value );
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, I didn't even know that we have this. Why do we have it and why do we need it?

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