You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/11/27 10:38:42 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request, #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
ppkarwasz opened a new pull request, #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147
This PR provides a simple Maven plugin with a `generate-location` goal, which process the compiled classes to add static location information.
The code is split into two modules: `log4j-instrument`, which contains the bytecode manipulation features and `log4j-maven-plugin` with the Maven plugin itself. It's less than 10 classes in total, so maybe the split is overkill.
Supported methods:
* all `Logger` methods are supported (and tested) except `catching`, `printf`, `throwing`, `traceEntry` and `traceExit`, which don't have a corresponding `LogBuilder` method,
* methods with a `(MessageSupplier)` and `(Supplier<?>)` signature are supported, but they are rewritten as `log(Supplier<Message>)` and `log(String, Supplier<?>...)` respectively due to the lack of corresponding `LogBuilder` methods. This introduces a small performance penalty and garbage, especially if the log statement is disabled.
The Maven plugin works for me (you can test with the project in `log4j-maven-plugin/src/it/location`), but it requires more testing (and unit tests) to be ready for inclusion.
All comments (naming conventions, implementation details) are welcome.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz closed pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz closed pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
URL: https://github.com/apache/logging-log4j2/pull/1147
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1364651560
@rgoers,
I have implemented support for all `Logger` methods, except `traceEntry/traceExit`. These need to create non-trivial lambdas, e.g.:
```lang-java
EntryMessage entry = logger.traceEntry(format, obj1, obj2)
```
would become
```lang-java
EntryMessage entry;
if (logger.isTraceEnabled()) {
entry = ... (equivalent to AbstractLogger.entryMsg) ...
logger.atTrace()
.withLocation(...)
.withMarker(AbstractLogger.ENTRY_MARKER)
.log(entry);
} else {
entry = null;
}
```
Do we even need location support for these methods? If so, it would be easier to add all least a part of the 10 `traceEntry/traceExit` methods to `LogBuilder`, but I would want to fill that interface with too many methods.
Otherwise this PR is ready to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1328241806
My current plan for (logging) world domination is:
1. Weave a project's bytecode to embed location information (this PR),
2. Weave bytecode that uses JUL, JCL, SLF4J to use `LogBuilder` with embedded location information,
3. Add support for weaving a project's dependencies,
4. Add support for converting source code (written using `Logger`, JUL, JCL or SLF4J) to use `LogBuilder`. If the source code uses `LogBuilder` adding location information is elegant and trivial (e.g. no need for local variables).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] jvz commented on a diff in pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#discussion_r1059812091
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:
##########
@@ -786,7 +786,7 @@ public LogEventProxy(final LogEvent event, final boolean includeLocation) {
this.thrownProxy = event.getThrownProxy();
this.contextData = memento(event.getContextData());
this.contextStack = event.getContextStack();
- this.source = includeLocation ? event.getSource() : null;
+ this.source = includeLocation || !event.isIncludeLocation() ? event.getSource() : null;
Review Comment:
`LogEventProxy` is not a relevant class in 3.x anymore due to https://issues.apache.org/jira/browse/LOG4J2-3228
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1377796115
This was merged in the [`logging-log4j-transform`](https://github.com/apache/logging-log4j-transform/commit/70e0328139cd3c72f04b0a75cb14daed7019bc70) repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
rgoers commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1368292772
Do you plan to merge this to master, 2.x or a separate repo? I can see valid reasons for each. A separate repo could allow us to introduce it independently as an alpha release.
The maven plugin is currently log4j-maven-plugin. While that might be OK I could see that possibly growing to include things like some sort of alternative to the annotation processor. If that isn't intended then the name should be more specfiic.
FWIW, having an alternative to the annotation processor might make sense in 3.0 since annotation processors have to be specifically declared when using the module path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1328638581
@rgoers,
The effect on debugging transformed code is similar to what happens with AspectJ or other weaving tools. If you enter multiple times into an `info(Marker, String, Throwable)` call, the debugger will transport you in sequence to `atInfo()`, `withLocation()`, `withMarker()`, `withThrowable()`, `log(String)`. Personally I don't find it more _strange_ than debugging a concatenation of strings, which redirects you to some method calls on `StringBuilder`.
That said, later on I can add a Javac plugin that generates the source code of the location cache, which resembles:
```
class MainClass$$Log4j$$Cache {
StackTraceElement[] locations = {
new StackTraceElement("org.example.MainClass", "method" "MainClass.java", 42),
...
};
}
```
As for the modified source code of the original class, I would rather use some source code refactoring tool (Carter suggested Error Prone) that would transform:
```
logger.info(MY_MARKER, "Hello world", e);
```
into
```
logger.atInfo().withMarker(MY_MARKER).withThrowable(e).log("Hello world");
```
After this transformation, we still need bytecode weaving, but the transformation consists in a trivial `withLocation()` call just after `atInfo()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1328659577
Regarding the methods not supported by this PR (or supported in a convoluted way):
- `catching` and `throwing` requires hardcoding the "Catching" and "Throwing" messages used in Log4j2 Core,
- `traceEntry` and `traceExit` require more work, maybe we can add them to `LogBuilder`,
- `printf` could be rewritten as a `Supplier<Message>` lambda that calls the appropriate message factory,
- apparently I missed `logMessage` that can be easily rewritten in terms of `LogBuilder` (although I don't expect it to be used in user code),
- support for `info(MessageSupplier)` is excessively convoluted: I need to wrap it in a new `Supplier<Message>` lambda. This could be simplified if either:
1. `MessageSupplier` were to extend `org.apache.logging.log4j.Supplier<Message>` (probably a non breaking change) or
2. `LogBuilder.log(org.apache.logging.log4j.Supplier<Message>` was replaced with `LogBuilder.log(java.util.function.Supplier<Message>`.
- support for `info(Supplier<?>)` is also not straightforward: I need to rewrite it as `info("{}", supplier)` first. This could be simplified if `LogBuilder.log(Supplier<Message>)` was replaced with `LogBuilder.log(Supplier<?>)` (+ some changes in the implementation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1368282380
@rgoers,
This version should cover all 380 methods in the `Logger` interface, when used directly.
Method references such as `List.of("Hello", "world").forEach(logger::info)` are **not** yet supported. I'll provide a separate PR for method references. If there are no objections I would like to merge this PR in a week or so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a diff in pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#discussion_r1032908020
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:
##########
@@ -786,7 +786,7 @@ public LogEventProxy(final LogEvent event, final boolean includeLocation) {
this.thrownProxy = event.getThrownProxy();
this.contextData = memento(event.getContextData());
this.contextStack = event.getContextStack();
- this.source = includeLocation ? event.getSource() : null;
+ this.source = includeLocation || !event.isIncludeLocation() ? event.getSource() : null;
Review Comment:
This line assumes that `event.getSource()` will not compute any location information if `isIncludeLocation()` is `false`. This is true for our implementations of `LogEvent`, but is not required by the specification (Javadoc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
rgoers commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1328567851
@ppkarwasz Would it be possible to also generate a text version of the .java file that matches the modified class file that could be used for debugging? I am guessing that a) that would be a lot of work and b) if we did that they we might as well redirect the compiler plugin to use them instead of generating a class file.
My only concern here will be how it impacts debugging of classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] rgoers commented on pull request #1147: [LOG4J2-3638] Provide a Maven plugin to inline location information
Posted by GitBox <gi...@apache.org>.
rgoers commented on PR #1147:
URL: https://github.com/apache/logging-log4j2/pull/1147#issuecomment-1364737537
Yes, I use those methods extensively and always want location info on them.
I am thinking that LogBuilder probably needs to be enhanced with addArg(). I know we discussed not adding it previously but that is really the only way to allow some arguments to use Suppliers while some do not. I suspect that would help in limiting the number of methods needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org