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.

Sign up for GitHub

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

Jump to bottom

[WIP] Ability to specify log category levels through separate options #35138

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

[WIP] Ability to specify log category levels through separate options #35138

wants to merge 13 commits into from

Conversation

vmuzikar
Copy link
Contributor

Closes #34957

The majority of changes focus on adding the support for option names with wildcard, e.g. log-level-<category>.

  • The wildcard can be placed anywhere in the name, e.g. if it's desired we could do something like log-<category>-level.
  • For env vars, the wildcard doesn't need any _ escaping. E.g. LOG_LEVEL_IO_QUARKUS=TRACE gets mapped to log-level-io.quarkus=TRACE.
  • Because of the ambiguity what _ represents in the env var format, the wildcard does not support - symbols, only a-z, 0-9 and . are generally accepted. Env vars match only A-Z, 0-9 and _.

What's missing and is to be done in this PR:

  • Docs
  • Tests

@stianst
Copy link
Contributor

stianst commented Nov 20, 2024

Some questions:

  • Format for environment variables is KC_LOG_LEVEL_<CATEGORY> where <CATEGORY> is upper-case and . replaced with _. For example KC_LOG_LEVEL_IO_QUARKUS=TRACE. Correct?
  • In keycloak.conf would it be log-level-io.quarkus=TRACE or log-level-io-quarkus=TRACE
  • Would this be supported as a command-line option as well? So, kc.sh start-dev --log-level-io-quarkus=trace?

Do the log category specific options take precedence over --log-level, so in the following example:

export KC_LOG_LEVEL_IO_QUARKUS=TRACE
kc.sh start-dev --log-level=io.quarkus:info

Would io.quarkus be set to TRACE?

@vmuzikar
Copy link
Contributor Author

vmuzikar commented Nov 20, 2024 •
edited
Loading

Format for environment variables is KC_LOG_LEVEL_<CATEGORY> where <CATEGORY> is upper-case and . replaced with _. For example KC_LOG_LEVEL_IO_QUARKUS=TRACE. Correct?

Correct.

In keycloak.conf would it be log-level-io.quarkus=TRACE or log-level-io-quarkus=TRACE

No, except for env vars, only . is accepted (apart from a-z0-9). So only log-level-io.quarkus=TRACE would be accepted for consistency.

Would this be supported as a command-line option as well? So, kc.sh start-dev --log-level-io-quarkus=trace?

Yes but only as kc.sh start-dev --log-level-io.quarkus=trace. --log-level-io-quarkus=trace would throw an unknown argument error (if used on CLI).

Do the log category specific options take precedence over --log-level

Yes, there's an explicit logic for that.

@vmuzikar
Copy link
Contributor Author

vmuzikar commented Nov 22, 2024 •
edited
Loading

@mabartos @shawkins Based on our offline discussion, updated the draft to map to quarkus.log.category."categories".level instead of setting the log level directly (as we did before). Happy to hear some feedback on this iteration. :)

@keycloak-github-bot
Copy link

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.saml.ArtifactBindingWithResolutionServiceTest#testReceiveArtifactLoginFullWithPost

Keycloak CI - Base IT (6)

jakarta.ws.rs.InternalServerErrorException: HTTP 500 Internal Server Error
	at org.jboss.resteasy.client.jaxrs.internal.ClientInvocation.handleErrorStatus(ClientInvocation.java:250)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.extractors.DefaultEntityExtractorFactory$3.extractEntity(DefaultEntityExtractorFactory.java:41)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invokeSync(ClientInvoker.java:136)
	at org.jboss.resteasy.client.jaxrs.internal.proxy.ClientInvoker.invoke(ClientInvoker.java:103)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot bot reviewed Nov 22, 2024
View reviewed changes
Copy link

@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@vmuzikar
Wildcard options
fec3b52 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
MappingFunction
9c79120 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Revert "MappingFunction"
4c1c4ff 
This reverts commit e4f539f.
@vmuzikar
Log level working
4362153 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Comments
ca918fa 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
uncrecognizedArgs handling
1ce9cf5 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Options now work
799ac27 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
strictExpectedValues(false)
af89822 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
comments
a2aa701 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Possible to map wildcard options to other options
6565e2b 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Mapping to quarkus.log.category."categories".level
142f185 
Signed-off-by: Václav Muzikář <[email protected]>
@vmuzikar
Bug and test fixes
06c101a 
Signed-off-by: Václav Muzikář <[email protected]>
mabartos
mabartos reviewed Nov 25, 2024
View reviewed changes
...runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java Outdated
Comment on lines 109 to 130
if (option.getKey() != null) {
Matcher matcher = WILDCARD_PLACEHOLDER_PATTERN.matcher(option.getKey());
if (matcher.find()) {
// Includes handling for both "--" prefix for CLI options and "kc." prefix
this.optionNameWildcardPattern = Pattern.compile("(?:" ARG_PREFIX "|kc\\.)" matcher.replaceFirst("([\\\\\\\\.a-zA-Z0-9] )"));

// Not using toEnvVarFormat because it would process the whole string incl the <...> wildcard.
Matcher envVarMatcher = WILDCARD_PLACEHOLDER_PATTERN.matcher(option.getKey().toUpperCase().replace("-", "_"));
this.envVarNameWildcardPattern = Pattern.compile("KC_" envVarMatcher.replaceFirst("([_A-Z0-9] )"));

if (to != null) {
toWildcardMatcher = WILDCARD_PLACEHOLDER_PATTERN.matcher(to);
if (!toWildcardMatcher.find()) {
throw new IllegalArgumentException("Attempted to map a wildcard option to a non-wildcard option");
}

this.toWildcardPattern = Pattern.compile(toWildcardMatcher.replaceFirst("([\\\\\\\\.a-zA-Z0-9] )"));
}
}

this.wildcardValuesTransformer = wildcardValuesTransformer;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@vmuzikar From the first sight, it looks good! But I didn't have time to look at it closer, but I'm curious if SmallRye Config supports something similar that could be used. As Quarkus supports specifying the log categories or multiple named datasources, they need to have some sort of functionality already implemented.

Did you have a chance to analyze it?

@vmuzikar
More fixes
050a9ee 
Signed-off-by: Václav Muzikář <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@mabartos mabartos mabartos left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
flaky-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ability to specify log category levels through separate options
3 participants
@vmuzikar @stianst @mabartos