mattcasters commented on code in PR #8343:
URL: https://github.com/apache/hop/pull/8343#discussion_r3999789518
##########
engine/src/main/java/org/apache/hop/core/HopEnvironment.java:
##########
@@ -246,4 +248,28 @@ public static void reset() {
HopClientEnvironment.reset();
initialized.set(null);
}
+
+ /**
+ * Silence noisy third-party loggers (such as Apache HttpClient 5 wire
logging) by defaulting
+ * their level to INFO if not explicitly configured, preventing verbose byte
dumping to console
+ * while preserving intentional debug flags. (Fixes #8340)
Review Comment:
**[nit]** The javadoc restates the method (default INFO, skip if explicit)
and appends `(Fixes #8340)`, which is ticket narration rather than a constraint
the next reader cannot see from the code. The catch at line 271 is documented
as “If Log4j2 core is not present” but also swallows any `Exception` (NPE,
misconfiguration, etc.).
**Suggestion:** Drop the issue id. Keep one sentence on why wire DEBUG is
unwanted. Catch `LinkageError` (and maybe `Exception` if you must) without
claiming the only failure mode is a missing `log4j-core`.
##########
engine/src/main/java/org/apache/hop/core/HopEnvironment.java:
##########
@@ -246,4 +248,28 @@ public static void reset() {
HopClientEnvironment.reset();
initialized.set(null);
}
+
+ /**
+ * Silence noisy third-party loggers (such as Apache HttpClient 5 wire
logging) by defaulting
+ * their level to INFO if not explicitly configured, preventing verbose byte
dumping to console
+ * while preserving intentional debug flags. (Fixes #8340)
+ */
+ private static void silenceVerboseThirdPartyLoggers() {
+ try {
+ org.apache.logging.log4j.core.LoggerContext context =
+ org.apache.logging.log4j.core.LoggerContext.getContext(false);
+ org.apache.logging.log4j.core.config.Configuration configuration =
context.getConfiguration();
+ String wireLoggerName = "org.apache.hc.client5.http.wire";
+ org.apache.logging.log4j.core.config.LoggerConfig loggerConfig =
+ configuration.getLoggerConfig(wireLoggerName);
+ boolean isExplicitlyConfigured =
+ wireLoggerName.equals(loggerConfig.getName()) &&
loggerConfig.getExplicitLevel() != null;
+ if (!isExplicitlyConfigured) {
+ org.apache.logging.log4j.core.config.Configurator.setLevel(
Review Comment:
**[bug]** `silenceVerboseThirdPartyLoggers()` only calls Log4j2
`Configurator.setLevel("org.apache.hc.client5.http.wire", INFO)`. Apache
HttpClient 5.6.4 does not log through Log4j2.
`DefaultManagedHttpClientConnection` binds `WIRE_LOG` with
`org.slf4j.LoggerFactory.getLogger("org.apache.hc.client5.http.wire")`, and
`Wire.wire()` emits only after `org.slf4j.Logger.isDebugEnabled()`. The Hop
client classpath has `slf4j-api` + `slf4j-nop` in `lib/core` and no
`log4j-slf4j2-impl`; `slf4j-nop` is the only `SLF4JServiceProvider` in the
assembled client. `log4j-core` being present does not make Log4j2 the SLF4J
backend. Issue #8340’s two-line records
(`org.apache.hc.client5.http.impl.Wire.wire()` then `DEBUG: http-outgoing-0 <<
...`) match JUL captured by an IDE console formatter, not Log4j2’s default
pattern (`%d [%t] %-5level %logger - %msg`). This Log4j2 mutation therefore
does not change HttpClient’s SLF4J `isDebugEnabled()` and does not silence JUL,
so it is unlikely to stop
the notification-poll dumps. Placement and “once at startup” are right; the
backend is still wrong.
**Suggestion:** Configure the backends that can actually print these
records, once at startup (same idea as `HopRun.silenceJulJdbcLoggers()`),
without touching `NotificationHttp`. Minimally: set JUL
`Logger.getLogger("org.apache.hc.client5.http.wire")` to `WARNING`/`INFO` (keep
a static reference; `LogManager` holds JUL loggers weakly) and keep the Log4j2
`getExplicitLevel()` guard for processes that do bind SLF4J to Log4j2. Do not
treat `-D` as creating a Log4j2 logger config — it does not. A default
`log4j2.xml` only helps if `log4j-slf4j2-impl` is the active provider.
##########
engine/src/test/java/org/apache/hop/core/HopEnvironmentSystemPropertyTest.java:
##########
@@ -94,4 +94,54 @@ void testConfigPropertyAppliedWhenNotSetViaCommandLine() {
actualValue,
"Config file property should be applied when not set via
command-line");
}
+
+ @Test
+ void testThirdPartyWireLoggingSilencedByDefault() throws Exception {
+ org.apache.logging.log4j.core.LoggerContext context =
+ org.apache.logging.log4j.core.LoggerContext.getContext(false);
+ org.apache.logging.log4j.core.config.Configuration configuration =
context.getConfiguration();
+ String wireLogger = "org.apache.hc.client5.http.wire";
+
+ try {
+ configuration.removeLogger(wireLogger);
+ context.updateLoggers();
+
+ HopEnvironment.init();
+
+ org.apache.logging.log4j.core.config.LoggerConfig lc =
+ configuration.getLoggerConfig(wireLogger);
+ assertEquals(
+ org.apache.logging.log4j.Level.INFO,
+ lc.getLevel(),
Review Comment:
**[suggestion]** Both new tests only assert
`configuration.getLoggerConfig(wireLogger).getLevel()` after
`HopEnvironment.init()`. That is the same “static level field” coverage the
previous review rejected: it never checks
`LoggerFactory.getLogger("org.apache.hc.client5.http.wire").isDebugEnabled()`,
never drives an HTTP exchange, and on the `hop-engine` test classpath SLF4J is
still `slf4j-nop` (`isDebugEnabled()` is always false). `getLoggerConfig` also
returns the parent/root when the named logger is missing, and the assertions
never check `lc.getName()`, so a root level of INFO would satisfy
`testThirdPartyWireLoggingSilencedByDefault` without a wire logger being added.
`finally` cleanup is an improvement and does not pollute other tests the way
the old `NotificationHttp` tests did.
**Suggestion:** Assert `wireLogger.equals(lc.getName())` as well as the
level. If the production fix stays on Log4j2, also assert
`org.apache.logging.log4j.LogManager.getLogger(wireLogger).isDebugEnabled()` is
false by default and true when DEBUG was set first. That still does not prove
HttpClient suppression on this module’s SLF4J binding; a useful regression test
belongs next to a real provider (or documents that this class only checks the
Log4j2 config object).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
