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

Documented quarkus.log.handler.file...rotation.max-file-size default value is incorrect #24041

Closed
jimbogithub opened this issue Mar 1, 2022 · 7 comments
Assignees
Labels

Comments

@jimbogithub
Copy link

Describe the bug

https://quarkus.io/guides/logging#quarkus-log-logging-log-config_quarkus.log.handler.file.-file-handlers-.rotation.max-file-size states the default size is 10. Based on the MemorySize definition, this would mean 10 bytes. Actual value from observed system appears to be 640K.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.5.1

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@jimbogithub jimbogithub added the kind/bug Something isn't working label Mar 1, 2022
@geoand
Copy link
Contributor

geoand commented Mar 2, 2022

@loicmathieu it seems like you updated RotationConfig last, so mind commenting?

@loicmathieu
Copy link
Contributor

Well, the documentation changes has been validated by @dmlloyd so he may be able to spot a mistake more than me.
I don't remember from where this 10 default values comes, I think it's 10MB and comes from the default from JBoss Logging.

@machi1990
Copy link
Member

machi1990 commented Mar 2, 2022

The issue is that the default value displayed on the generated documentation is only for visualisation (see https://github.com/quarkusio/quarkus/blob/main/core/runtime/src/main/java/io/quarkus/runtime/logging/FileConfig.java#L61). The true default value will be whatever that is assigned by JBoss Logging, which is 640K. It could the case that the default value in upstream was updated and the change was not done on quarkus side or that at some point in time we moved from using defaultValue to defaultValueDocumentation for this configuration knob

Two solutions that I see here:

  1. Change the ConfigItem to using defaultValue instead of defaultValueDocumentation.
  2. Change the default documentation value to match the upstream one

I am in favor of 1 so that any upstream default change does not affect the default value we apply and display on our docs.

@gsmet
Copy link
Member

gsmet commented Mar 2, 2022

@machi1990 yeah I think it makes more sense to set it to 10 MB.

@VanillaSpoon
Copy link
Contributor

Hey :) could i be assigned this issue. I'll get working on solution 1.

@machi1990 machi1990 added the good first issue Good for newcomers label Mar 2, 2022
@machi1990
Copy link
Member

@machi1990 yeah I think it makes more sense to set it to 10 MB.

+1

Hey :) could i be assigned this issue. I'll get working on solution 1.

Thanks @VanillaSpoon consider the issue yours. What option 1 is to change the

@ConfigItem(defaultValueDocumentation = "10")

from line https://github.com/quarkusio/quarkus/blob/main/core/runtime/src/main/java/io/quarkus/runtime/logging/FileConfig.java#L61
to

@ConfigItem(defaultValue = "10M")

@machi1990
Copy link
Member

Fixed by #25072

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants