Respect custom ConfigRoot.name in ConfigInstantiator & fix minLogLevel handling #24432
Conversation
| @@ -60,7 +61,16 @@ public static void handleObject(Object o) { | |||
| } | |||
|
|
|||
| final Class<?> cls = o.getClass(); | |||
| final String name = dashify(cls.getSimpleName().substring(0, cls.getSimpleName().length() - clsNameSuffix.length())); | |||
There was a problem hiding this comment.
Before this change, the following didn't apply the actual config:
because
quarkus.log-build-time.min-level was looked up, which obviously doesn't exist.
| if (configRoot != null && !configRoot.name().equals(ConfigItem.HYPHENATED_ELEMENT_NAME)) { | ||
| name = configRoot.name(); | ||
| if (name.startsWith("<<")) { | ||
| throw new IllegalArgumentException("Found unsupported @ConfigRoot.name = " + name + " on " + cls); |
There was a problem hiding this comment.
I had a quick look at other default names and didn't see how to support them properly. So better complain loudly here.
There was a problem hiding this comment.
In which case do we have a root that is not in the hyphenated format? (I think it makes sense to protect that case, just wondering).
There was a problem hiding this comment.
We have a few that are using @ConfigRoot.name = ConfigItem.PARENT, e.g. CommandLineRuntimeConfig.
But those are very special and it doesn't make sense to pass them to ConfigInstantiator, AFAICS.
| final Level logLevel = getLogLevel(categoryName, config, categories, buildConfig.minLevel); | ||
| final Level minLogLevel = buildCategory == null | ||
| ? buildConfig.minLevel | ||
| : buildCategory.minLevel.getLevel(); |
There was a problem hiding this comment.
Now that logBuildTimeConfig is loaded properly, buildCategory.minLevel.getLevel() would fail due to:
(in case the min-level is not set directly for the given category)
So, I had to use the same inheritance-aware approach as for the actual log level (and/or the min-level in LoggingResourceProcessor):
| return rootMinLevel; | ||
| } | ||
| categoryName = categoryName.substring(0, lastDotIndex); | ||
| } |
There was a problem hiding this comment.
I've rewritten the alorithm that goes up the category path to:
- use it for different
Configobjects (hence thelevelExtractor) - fix a gap in the original implementation that was falling back to the root (min-)level too early (e.g. if
com.foo.barhas.levelbut nomin-level, then we must continue withcomin casecom.foois not configured)
/cc @galderz
core/runtime/src/main/java/io/quarkus/runtime/logging/LoggingSetupRecorder.java
Outdated
Show resolved
Hide resolved
|
@stuartwdouglas in context of this, it's unclear to me how that's supposed to work: given that BannerRuntimeConfig does not define a name for @ConfigRoot and AFAICS, there is no banner-runtime infix.
|
This comment has been minimized.
This comment has been minimized.
|
Thanks @radcortez! |
core/runtime/src/main/java/io/quarkus/runtime/logging/LoggingSetupRecorder.java
Outdated
Show resolved
Hide resolved
famod
added
the
triage/waiting-for-ci
quarkus-bot
bot
removed
the
triage/waiting-for-ci
Reported via #24406 (but it's not the main issue)
I'll add some notes to the changed code...