Implement the logger-log4j module using log4j apis directly
#756
Conversation
This is meant to avoid slf4j indirection, and use a code-path with better test coverage within log4j itself. For example, this implementation will avoid the unintended allocations incurred by LOG4J2-3560. This module uses a bridge with dynamic priority based on the presence of log4j-core. In the case that log4j-core is present, it's preferred over the default.
Generate changelog in
|
| @@ -1 +1,4 @@ | |||
| org.gradle.parallel=true | |||
| # Avoid formatting copyrights, which are formatted in a | |||
| # slightly non-standard way due to javapoet codegen. | |||
| com.palantir.baseline-format.copyright=false | |||
There was a problem hiding this comment.
This will opt us out of unintentionally formatting generated classes that we check in
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
This block was added by robots, shouldn't happen anymore
There was a problem hiding this comment.
Overall LGTM, thanks for putting this together @carterkozak ! Left a few comments/questions
| @@ -0,0 +1,61 @@ | |||
| /* | |||
| * (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. | |||
There was a problem hiding this comment.
nit:
| * (c) Copyright 2021 Palantir Technologies Inc. All rights reserved. | |
| * (c) Copyright 2022 Palantir Technologies Inc. All rights reserved. |
versions.props
Outdated
| @@ -16,4 +16,5 @@ com.google.errorprone:error_prone_refaster = 2.3.3 | |||
|
|
|||
| # Don't automatically force everything to upgrade slf4j | |||
| org.slf4j:slf4j-api = 1.7.31 | |||
| org.apache.logging.log4j:* = 2.17.2 | |||
There was a problem hiding this comment.
Are we intentionally pinning on 2.17.2 for older support? Should we add a comment about when we'd want to unpin? 2.18.0 has been out for about a month now and making its way through automated upgrades.
There was a problem hiding this comment.
Initially I hadn't used a compileOnly dependency, the goal here was to avoid pushing framework upgrades faster than our standard machinery might intend (which don't pin back releases, in fact the new version was bundled into products within hours of the release)
There was a problem hiding this comment.
Updated using the latest release, un-pinned
| Logger facades delegate to logger implementations. Unfortunately we | ||
| cannot opt out of specific custom rules, otherwise we would only | ||
| opt out of 'BanLoggingImplementations'. | ||
| --> | ||
| <suppress files="[/\\]logger-.*Bridge\.java" checks="IllegalImport" /> |
There was a problem hiding this comment.
I'm fine with this suppression given this project is explicitly is dealing with loggers, but one workaround would be to use/generate fully qualified type names where used for the things we consider illegal so they aren't imported.
There was a problem hiding this comment.
That's an option, but it would impact readability of the generated code, so I opted against it.
|
Released 1.28.0 |
This is meant to avoid slf4j indirection, and use a code-path
with better test coverage within log4j itself. For example,
this implementation will avoid the unintended allocations
incurred by LOG4J2-3560.
This module uses a bridge with dynamic priority based on the
presence of log4j-core. In the case that log4j-core is present,
it's preferred over the default.
In practice, we will ship this as a runtime dependency of our internal
logging framework, invisible to consumers.
After this PR
==COMMIT_MSG==
Implement the
logger-log4jmodule using log4j apis directly==COMMIT_MSG==
Possible downsides?
More to maintain