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

TRACE logging improvements #12938

Closed
galderz opened this issue Oct 26, 2020 · 3 comments · Fixed by #13376
Closed

TRACE logging improvements #12938

galderz opened this issue Oct 26, 2020 · 3 comments · Fixed by #13376
Assignees
Milestone

Comments

@galderz
Copy link
Member

galderz commented Oct 26, 2020

Infinispan uses this idiom where it caches whether trace is enabled in a static final field. In native testing it has been shown that this field is true even if trace has not been enabled. @Sanne already predicted the issue. The linked issue also explains the reason why this happens. In Hibernate's case, they worked around it by making the cached field non static.

After much discussion, an agreement has been reached that isTraceEanbled calls should be computed at build time (only enable it if it has been configured) and if not enabled, all the code that adds/maintains trace logging should be dead code eliminated if possible. This way, for code that uses isTraceEnabled to return true, it would require using a native executable that has been built with this option enabled. If this option was not enabled, the native executable would remove code to isTraceEnabled true paths, reducing its size both in terms of code and strings used within it.

Once this is in place, Hibernate could go back to having TRACE enabled check cached as static field and you could get the benefits above. The recommendation would be for instance or to use on the fly isTraceEnabled(). By folding this colds, they'd precomputed at build time and replaced, so no need for static fields. In fact, static fields caching trace enabled should be avoided since they'll always return true.

This change would require an update to the Quarkus logging documentation.

@quarkusbot
Copy link

/cc @kenfinnigan, @Ladicek

@galderz
Copy link
Member Author

galderz commented Oct 26, 2020

The solution described can be achieved by using gizmo to generate bytecode at build time for a class that substitutes BasicLogger.isTraceEnabled method, if any categories have been enabled for TRACE. Then, the substituted method would check whether the log name is equals too, or starts with, the category to trace in which case it would return true. Then, by adding @Fold annotation, it should force these isTraceEnabled calls to happen at build time hence enabling dead code elimination.

@galderz
Copy link
Member Author

galderz commented Nov 4, 2020

Updated the description to better match the expected behaviour. PR incoming.

galderz added a commit to galderz/quarkus that referenced this issue Nov 4, 2020
galderz added a commit to galderz/quarkus that referenced this issue Dec 15, 2020
* It enables log levels below that to be folded in native.
* Runtime min-level has been removed (was deprecated already).
galderz added a commit to galderz/quarkus that referenced this issue Dec 21, 2020
* It enables log levels below that to be folded in native.
* Runtime min-level has been removed (was deprecated already).
gsmet pushed a commit to galderz/quarkus that referenced this issue Jan 4, 2021
* It enables log levels below that to be folded in native.
* Runtime min-level has been removed (was deprecated already).
@ghost ghost added this to the 1.11 - master milestone Jan 5, 2021
cemnura pushed a commit to cemnura/quarkus that referenced this issue Jan 20, 2021
* It enables log levels below that to be folded in native.
* Runtime min-level has been removed (was deprecated already).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants