[
https://issues.apache.org/jira/browse/GROOVY-12378?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113091#comment-18113091
]
ASF GitHub Bot commented on GROOVY-12378:
-----------------------------------------
codecov-commenter commented on PR #2901:
URL: https://github.com/apache/groovy/pull/2901#issuecomment-5594302308
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2901?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `29.33333%` with `53 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 71.1780%. Comparing base
([`2290229`](https://app.codecov.io/gh/apache/groovy/commit/229022901454900a97368afcd55ba4e2571983bb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`994c78c`](https://app.codecov.io/gh/apache/groovy/commit/994c78c517561ca817f6d052ec10d97ed90012c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2901?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[src/main/java/groovy/util/logging/Log4j2.java](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Futil%2Flogging%2FLog4j2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvdXRpbC9sb2dnaW5nL0xvZzRqMi5qYXZh)
| 0.0000% | [30 Missing :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
|
[...odehaus/groovy/transform/LogASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FLogASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9Mb2dBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| 48.8889% | [19 Missing and 4 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2901 +/- ##
==================================================
- Coverage 71.2116% 71.1780% -0.0336%
+ Complexity 37622 37609 -13
==================================================
Files 1581 1581
Lines 136152 136226 +74
Branches 25291 25304 +13
==================================================
+ Hits 96956 96963 +7
- Misses 30462 30519 +57
- Partials 8734 8744 +10
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2901?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...odehaus/groovy/transform/LogASTTransformation.java](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Ftransform%2FLogASTTransformation.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3RyYW5zZm9ybS9Mb2dBU1RUcmFuc2Zvcm1hdGlvbi5qYXZh)
| `55.9006% <48.8889%> (-3.0737%)` | :arrow_down: |
|
[src/main/java/groovy/util/logging/Log4j2.java](https://app.codecov.io/gh/apache/groovy/pull/2901?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Futil%2Flogging%2FLog4j2.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvdXRpbC9sb2dnaW5nL0xvZzRqMi5qYXZh)
| `0.0000% <0.0000%> (ø)` | |
... and [9 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2901/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> @Log4j2: staticLocation option to emit compile-time caller locations via
> LogBuilder.withLocation
> ------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12378
> URL: https://issues.apache.org/jira/browse/GROOVY-12378
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Follow-up to GROOVY-12354. Logging frameworks locate the caller of a logging
> statement by walking the stack, and every frame Groovy's runtime inserts
> between the statement and the logger makes that answer wrong:
> * on a JVM, whenever a call is dispatched through the metaclass rather than
> linked at the call site: the reflective cold tier when enabled, a dynamic
> method name ({{log."$level"(msg)}}), an explicit {{invokeMethod}};
> * in a GraalVM native image, always: GraalVM's method-handle interpreter
> frames are visible to {{StackWalker}} and stack traces in both dispatch modes.
> {{java.util.logging}} and Logback have package skip lists that cover this
> (documented in the invokedynamic guide). Log4j2 has no equivalent hook, and
> its tracker shows it never will: every wrapper-related report (LOG4J2-555,
> LOG4J2-1028, LOG4J2-2975, discussions #2133 and #2243) is answered with "pass
> the boundary FQCN" or "use {{LogBuilder.withLocation}}", and wrappers are
> discouraged outright. The FQCN boundary cannot express Groovy's case, because
> the runtime frames sit *after* the last logger frame. The other answer fits
> exactly: {{LogBuilder.withLocation(StackTraceElement)}} lets the caller
> supply the location, and Log4j2's own LOG4J2-1449 endorsed compile-time
> locations when Scala asked for them (it stalled only on a garbage-free API
> shape, which Groovy does not need).
> The {{@Log4j2}} AST transform knows the class, method, source file and line
> of every logging statement it rewrites, so it can supply the location at
> compile time.
> h3. Proposal
> Add {{boolean staticLocation() default false}} to
> {{groovy.util.logging.Log4j2}}. When set,
> {{Log4j2LoggingStrategy.wrapLoggingMethodCall}} emits the builder form
> instead of the guarded direct call:
> {code:java}
> // today
> if (log.isInfoEnabled()) log.info(msg)
> // with staticLocation = true
> log.atInfo().withLocation($LOC$3).log(msg)
> {code}
> where {{$LOC$n}} is a {{private static final StackTraceElement}} per call
> site, initialised from the annotated class name, enclosing method name,
> source file name and line of the statement. No allocation or stack walk
> happens at run time; the builder's own level check replaces the
> {{isXxxEnabled}} guard.
> Overload mapping onto {{LogBuilder}}: message and parameterised {{log(String,
> Object...)}} as-is; {{Marker}} first argument via {{withMarker}}; trailing
> {{Throwable}} via {{withThrowable}}; {{Supplier}}, {{Message}},
> {{CharSequence}} and {{Object}} arguments onto the corresponding {{log(...)}}
> overloads. Statements the strategy does not rewrite today (see
> {{LogASTTransformation.usesSimpleMethodArgumentsOnly}}) keep their current
> handling.
> Compatibility: {{LogBuilder}} exists since Log4j2 2.13 (2019);
> {{withLocation(StackTraceElement)}} is an interface default that is a no-op
> for foreign implementations, so it degrades to today's behaviour rather than
> failing. If {{staticLocation = true}} and
> {{org.apache.logging.log4j.LogBuilder}} cannot be resolved on the compile
> classpath, the transform must report a compilation error, not silently emit
> the old shape: an opt-in that does nothing would be worse than none.
> h3. Why opt-in
> The generated code is arguably better for every user: supplying the location
> removes Log4j2's runtime stack walk, the expensive part of any
> {{%C}}/{{%M}}/{{%F}}/{{%L}} layout, and fixes the metaclass-routed shapes on
> the JVM that the GROOVY-12354 default flip does not reach. It is nevertheless
> gated because the call shape changes (tests that mock the logger or inspect
> generated bytecode will notice), because of the 2.13 floor, and because a
> behaviour change to a widely used transform belongs in a point release as an
> option first. Flipping the default, and a {{CompilerConfiguration}} switch so
> a native build can enable it project-wide, are follow-ups once it has seen
> use.
> h3. Scope
> Only statements generated by the transform. A hand-declared logger field
> keeps today's behaviour, for which the documented settings remain the answer.
> {{@Log}}, {{@Slf4j}} and {{@Commons}} have no equivalent API (SLF4J's
> {{CallerBoundaryAware}} is a boundary, not a location) and are out of scope.
> h3. Tests and docs
> * Transform tests: generated shape with and without the attribute; each
> overload mapping; one {{StackTraceElement}} per call site with the
> statement's line, not the class or method line; the compile error when
> {{LogBuilder}} is absent.
> * Behavioural test with {{log4j-core}}: a {{%C.%M(%F:%L)}} layout reports the
> annotated class and statement line when the call is made through
> {{log."$level"(msg)}}, which is wrong without the option on every Groovy
> version.
> * Document the attribute on {{@Log4j2}} and add it to the "Caller location"
> section of the invokedynamic guide, including the native-image subsection, as
> the recommended setting for Log4j2 users there.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
