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/12/27 20:59:07 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new pull request, #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
ppkarwasz opened a new pull request, #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171
This PR adds a minimal amount of methods to greatly simplify flow tracing with `LogBuilder`. E.g.:
```
logger.traceEntry("a={}, b={}", 1, 42);
```
can be now rewrittent as:
```
logger.atTrace()
.withMarker(AbstractLogger.ENTRY_MARKER)
.logAndGet(() -> {
LoggingSystem.getFlowMessageFactory()
.newEntryMessage("a={}, b={}", 1, 42);
});
```
The latter can be easily perfomed by a bytecode manipulation tool (cf. #1147).
It also refactors the factory methods that create `EntryMessage/ExitMessage`s to be low-garbage.
--
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] garydgregory commented on pull request #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#issuecomment-1367254944
I've never been a fan of using "And" in method names, but no great alternative comes to mind.
Shouldn't new public methods have Java since tags?
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#discussion_r1059090120
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/FlowMessageFactory.java:
##########
@@ -30,6 +40,25 @@ public interface FlowMessageFactory {
*/
EntryMessage newEntryMessage(Message message);
+ /**
+ * Creates a new exit message based on a return value and a forma string.
+ *
+ * @param format a format string
+ * @param result the return value
+ * @return the new exit message
+ * @since 2.20
+ */
+ ExitMessage newExitMessage(String format, Object result);
Review Comment:
I am not sure what the default implementation should be.
Anyway a minor version bump allows to break a SPI.
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#discussion_r1059155588
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/FlowMessageFactory.java:
##########
@@ -30,6 +40,25 @@ public interface FlowMessageFactory {
*/
EntryMessage newEntryMessage(Message message);
+ /**
+ * Creates a new exit message based on a return value and a forma string.
+ *
+ * @param format a format string
+ * @param result the return value
+ * @return the new exit message
+ * @since 2.20
+ */
+ ExitMessage newExitMessage(String format, Object result);
Review Comment:
That sounds reasonable. Sometimes, a default implementation could be to either return `null` or just `throw new UnsupportedOperationException()` with appropriate checks on the consuming end as appropriate.
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#discussion_r1059787265
##########
log4j-api/src/main/java/org/apache/logging/log4j/Logger.java:
##########
@@ -1686,6 +1687,14 @@ void fatal(String message, Object p0, Object p1, Object p2, Object p3, Object p4
*/
<MF extends MessageFactory> MF getMessageFactory();
+ /**
+ * Gets the flow message factory used to convert messages into flow messages.
+ *
+ * @return the flow message factory, as an instance of {@link FlowMessageFactory}.
+ * @since 2.20
+ */
+ FlowMessageFactory getFlowMessageFactory();
Review Comment:
Just wanted to say thanks for handling this here as I was also adding this field in 3.x, so that simplified things for me!
--
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 merged pull request #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
ppkarwasz merged PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on code in PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#discussion_r1057930722
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/LoggerTest.java:
##########
@@ -203,11 +191,18 @@ public String get() {
}
});
logger.traceExit(msg, 3);
- assertEquals(2, results.size());
- assertThat("Incorrect Entry", results.get(0), startsWith("ENTER[ FLOW ] TRACE Enter"));
- assertThat("Missing entry data", results.get(0), containsString("doFoo(a=1, b=2)"));
- assertThat("Incorrect Exit", results.get(1), startsWith("EXIT[ FLOW ] TRACE Exit"));
- assertThat("Missing exit data", results.get(1), containsString("doFoo(a=1, b=2): 3"));
+ assertThat(results).hasSize(2);
+ assertThat(results.get(0)).startsWith("ENTER[ FLOW ] TRACE Enter").contains("doFoo(a=1, b=2)");
+ assertThat(results.get(1)).startsWith("EXIT[ FLOW ] TRACE Exit").contains("doFoo(a=1, b=2): 3");
+ }
+
+ @Test
+ public void flowTracingNoFormat() {
+ logger.traceEntry(null, 1, "2", new ObjectMessage(3));
+ logger.traceExit((String) null, 4);
+ assertThat(results).hasSize(2);
+ assertThat(results.get(0)).isEqualTo("ENTER[ FLOW ] TRACE Enter with params: [1, 2, 3]");
+ assertThat(results.get(1)).isEqualTo("EXIT[ FLOW ] TRACE Exit with: 3");
Review Comment:
The previous code printed "Enter param(1, 2, 3)", but used a format that depended on the number of parameters. The current code uses a `ParameterizedMessage("params: {}", (Object) params)`.
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#issuecomment-1367265032
@garydgregory,
This is not a breaking change, so I added `@since 2.20` tags.
--
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 #1171: [LOG4J2-3645] Minimal changes to allow flow tracing with LogBuilder
Posted by GitBox <gi...@apache.org>.
jvz commented on code in PR #1171:
URL: https://github.com/apache/logging-log4j2/pull/1171#discussion_r1059041025
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/FlowMessageFactory.java:
##########
@@ -30,6 +40,25 @@ public interface FlowMessageFactory {
*/
EntryMessage newEntryMessage(Message message);
+ /**
+ * Creates a new exit message based on a return value and a forma string.
+ *
+ * @param format a format string
+ * @param result the return value
+ * @return the new exit message
+ * @since 2.20
+ */
+ ExitMessage newExitMessage(String format, Object result);
Review Comment:
Should these be default methods, or is it ok to break the API here?
--
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