Make isTraceEnabled calls be computed at build time #12938

[galderz]

Closes #12938

Logging documentation guide still needs updating, but I've tried to make the javadoc CategoryBuildTimeConfig as explicit as possible.

In essence, this change pushes all isTraceEnabled calls to be computed at build time making them eagerly folded. This means that if no build time trace is enabled, all runtime isTraceEnabled calls will be precomputed to false, leading to the true branches to be dead code eliminated (along with any strings created inside of them). Whether those return true/false are controlled by the newly added build time configuration option.

The only exception are those isTraceEnabled calls made in static blocks or initializing static fields. These will always return true and that's not changing. Any users of isTraceEnabled caching in static fields should be changed.

Note that log.trace() still works as usual. This means that any messages passed are only printed if runtime log level is TRACE.

Finally, note that I've changed slightly the name from my suggestion in here.

TODO:

• Update documentation (once PR approved).

[Sanne]

This is a bit odd in terms of semantics, no? Wouldn't it be far easier for users to understand this feature if the focus was about the trace cathegory as a whole not being something which can be controlled at runtime?

I love the potential of your patch, but in the current shape I'm a bit concerned about it exposing much complexity.

[galderz]

Yeah, it feels a bit odd. I already hinted at this issue here, but @dmlloyd opposed the idea that build time trace configuration should delete all trace calls.

To make this a fully build time feature, aside of what I did in this PR, you'd need to remove quarkus.log.category.<...>.level=TRACE option, meaning the ability to set TRACE as a runtime log level.

[galderz]

Note that as @dmlloyd said, an option can only either be build time or runtime.

[Sanne]

To make this a fully build time feature, aside of what I did in this PR, you'd need to remove quarkus.log.category.<...>.level=TRACE option, meaning the ability to set TRACE as a runtime log level.

Right, I can see how people would find that odd as well. But personally I think that would be better: this kinda needs a re-definition of a Level to be special.

Ideally the format of configuration could be more specific; allowing the user to choose which runtime configuration elements are made available via an enumeration - I proposed this on the list in the past.

My past proposal was expressed in the area of databases; so in the build time configuration one could, for example, leave MySQL out:

supported_databases={H2,Oracle,PostgreSQL}

then at runtime one has to choose ONE among the enumeration.

Similarly for log levels one could have

< >.level={TRACE,DEBUG,INFO}

or choose

< >.level={DEBUG,INFO}

Which would leave the TRACE option out, exluding it from the valid runtime options and therefore folding it as "false" - consistently in all circumstances.

IMO it has the following benefits:

1. it's clear what the end user is opting for
2. people who prefer having more options at runtime can have their way if they want to
3. Clean, precise separation of build-time vs runtime configuration

[dmlloyd]

We had effectively this feature in the min-level configuration. Filtering out arbitrary levels doesn't seem as specifically useful as setting a single bound (and might lead to some confusion). It certainly would be more complex to implement.

[galderz]

@dmlloyd raises a valid point here. Although it sounds useful, it also opens up to some potential misuse, e.g. what if the user sets TRACE,WARN, do you really only want TRACE and WARN and not DEBUG, INFO? If you want to handle ranges, you could add support for .. as an option.

min-level might make more sense, from level X upward. Why was that removed? Was the name confusing? Do you need the flexibility of the enumeration?

[Sanne]

I agree, min-level looks good. I don't know why it was removed.

[galderz]

Looking at LogConfig, min-level seemed to be a runtime option. It got deprecated in #10672, but at first glance I don't see specific reasons on why it got deprecated.

[dmlloyd]

It was a build time option originally. It was removed because nobody used it or showed much interest in it.

[galderz]

I'm working on an update to the PR.

[galderz]

Please see #13376 for a follow up PR with a new solution based on the feedback here.