Modernize logger usage check

[andrross]

Accept a trailing throwable with parameters as opposed to requiring a clunkly lambda to use this syntax.

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[andrross]

This class was arbitrarily chosen just to demonstrate the syntax the previous logger usage checked forced us to use and the simpler alternative.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Modernize logger usage in TransportService Relevant files: server/src/main/java/org/opensearch/transport/TransportService.java Sub-PR theme: Support trailing Throwable argument in logger usage checker Relevant files: test/logger-usage/src/main/java/org/opensearch/test/loggerusage/OpenSearchLoggerUsageChecker.java test/logger-usage/src/test/java/org/opensearch/test/loggerusage/OpenSearchLoggerUsageTests.java

⚡ Recommended focus areas for review Possible Issue checkFailTrailingThrowableTooFewPlaceholders calls logger.info("Hello", "world", new Exception()) which has 0 placeholders, 2 positional args, and a trailing Throwable. With lastArgIsThrowable=true and positionalArgsLength=2, the check logMessageLength.minValue == positionalArgsLength - 1 becomes 0 == 1, which is false, so it correctly fails. However, consider the case logger.info("Hello {}, {}, {}", "a", new Exception()): placeholders=3, positionalArgsLength=2, lastArgIsThrowable=true. The condition 0 != 2 triggers the wrong-usage callback, and 3 == 2-1 is false, so it still reports incorrectly. This case (too few args for placeholders with a trailing Throwable) is handled, but the error message ("Expected 3 arguments but got 2") may be confusing since the user provided 2 args + 1 throwable. Confirm intended messaging here. if (logMessageLength.minValue != positionalArgsLength) { // SLF4J / Log4j2 style: a trailing Throwable is not a placeholder argument. When the last argument is // statically a Throwable, Log4j2 extracts it as the logged exception, so the message is allowed to have // one fewer placeholder than the number of positional arguments. if (lastArgIsThrowable && logMessageLength.minValue == positionalArgsLength - 1) { return; } wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, "Expected " + logMessageLength.minValue + " arguments but got " + positionalArgsLength)); return; } Class Resolution Side Effects isThrowable uses Class.forName(className, false, classLoader) which can trigger class loading of the target class's supertypes. If a class on the analyzed classpath references types not available to the checker's classloader (even just for the supertype chain of the argument's static type), Class.forName may throw NoClassDefFoundError, which is caught here, but the cache will then permanently mark the class as not-a-throwable. If the same class name later becomes resolvable in a different context (unlikely in a single run, but possible across checker invocations sharing the static cache), the negative result persists. Since THROWABLE_CACHE is static, results leak across checker instances/JVM reuse (e.g., Gradle daemon). private static final Map<String, Boolean> THROWABLE_CACHE = new ConcurrentHashMap<>(); private final ClassLoader classLoader; ThrowableInterpreter(ClassLoader classLoader) { super(Opcodes.ASM7); this.classLoader = classLoader == null ? ThrowableInterpreter.class.getClassLoader() : classLoader; } @Override public BasicValue newValue(Type type) { if (type != null && type.getSort() == Type.OBJECT && isThrowable(type)) { return new ThrowableBasicValue(type); } return super.newValue(type); } @Override public BasicValue merge(BasicValue value1, BasicValue value2) { if (value1.equals(value2)) { return value1; } if (value1 instanceof ThrowableBasicValue && value2 instanceof ThrowableBasicValue) { // Both branches yield a throwable, but possibly of different concrete types: keep tracking it as a // throwable using the common Throwable supertype. return new ThrowableBasicValue(THROWABLE_CLASS); } Type type1 = value1.getType(); Type type2 = value2.getType(); if (isReference(type1) && isReference(type2)) { // Different reference types (at least one of which is not a throwable) merge to a plain object // reference, mirroring the default BasicInterpreter behavior of normalizing references to Object. return BasicValue.REFERENCE_VALUE; } return super.merge(value1, value2); } private static boolean isReference(Type type) { return type != null && (type.getSort() == Type.OBJECT || type.getSort() == Type.ARRAY); } private boolean isThrowable(Type type) { return THROWABLE_CACHE.computeIfAbsent(type.getClassName(), className -> { try { Class<?> clazz = Class.forName(className, false, classLoader); return Throwable.class.isAssignableFrom(clazz); } catch (Throwable t) { // If the class (or one of its supertypes) cannot be resolved on the checker's class path we // conservatively treat it as not a throwable. This keeps the previous, stricter behavior for // those cases rather than risking a false positive. return false; } }); } } Resource Leak URLClassLoader created in buildClassLoader is never closed. While the process is typically short-lived, this still leaks file handles for each class directory until JVM exit, which can matter when the checker runs many times within a long-lived Gradle daemon. private static ClassLoader buildClassLoader(String... classDirectories) { List<URL> urls = new ArrayList<>(); for (String classDirectory : classDirectories) { try { urls.add(Paths.get(classDirectory).toUri().toURL()); } catch (MalformedURLException e) { throw new IllegalArgumentException("Unable to resolve class directory " + classDirectory, e); } } return new URLClassLoader(urls.toArray(new URL[0]), OpenSearchLoggerUsageChecker.class.getClassLoader()); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid sharing classloader-dependent cache statically THROWABLE_CACHE is a static field shared across all ThrowableInterpreter instances, but the resolution depends on the per-instance classLoader. Different invocations with different class loaders may produce inconsistent results from the cache. Either make the cache an instance field or key it by both class name and class loader. test/logger-usage/src/main/java/org/opensearch/test/loggerusage/OpenSearchLoggerUsageChecker.java [759-771] +private final Map<String, Boolean> throwableCache = new ConcurrentHashMap<>(); + private boolean isThrowable(Type type) { - return THROWABLE_CACHE.computeIfAbsent(type.getClassName(), className -> { + return throwableCache.computeIfAbsent(type.getClassName(), className -> { try { Class<?> clazz = Class.forName(className, false, classLoader); return Throwable.class.isAssignableFrom(clazz); } catch (Throwable t) { return false; } }); } Suggestion importance[1-10]: 7 __ Why: Valid concern: the static THROWABLE_CACHE is keyed only by class name but resolution depends on the per-instance classLoader, which can produce inconsistent or incorrect results across instances with different class loaders.	Medium
Possible issue	Fix operator precedence in conditional check The ternary operator has lower precedence than &&, so this condition is parsed as (A && B) ? C : true, which evaluates incorrectly and always falls through when lengthWithoutMarker == 2. Wrap the original && clause in parentheses to restore the intended logic. test/logger-usage/src/main/java/org/opensearch/test/loggerusage/OpenSearchLoggerUsageChecker.java [411-412] } else if ((lengthWithoutMarker == 1 || lengthWithoutMarker == 2) && - lengthWithoutMarker == 2 ? argumentTypes[markerOffset + 1].equals(THROWABLE_CLASS) : true) { + (lengthWithoutMarker == 2 ? argumentTypes[markerOffset + 1].equals(THROWABLE_CLASS) : true)) { Suggestion importance[1-10]: 2 __ Why: The flagged code is pre-existing and not modified in this PR's diff (the lines were not added by this PR). While the precedence concern may have technical merit, it is outside the scope of this PR's changes.	Low

[github-actions]

✅ Gradle check result for eaa4c8b: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.39%. Comparing base (319d78a) to head (eaa4c8b).

Files with missing lines	Patch %	Lines
...ava/org/opensearch/transport/TransportService.java	12.50%	7 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22351      +/-   ##
============================================
- Coverage     73.40%   73.39%   -0.02%     
- Complexity    76075    76096      +21     
============================================
  Files          6076     6076              
  Lines        345500   345490      -10     
  Branches      49732    49732              
============================================
- Hits         253608   253565      -43     
- Misses        71707    71731      +24     
- Partials      20185    20194       +9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cwperks]

I'm trying to understand why we need to do all the convoluted logic with classloader here, is this because of the plugin ecosystem?

[andrross]

It's because the final argument may be an exception type defined in the OpenSearch code base, so we need to be able to load those classes in order to do the check.

[andrross]

Previously the check could operate with just the JDK and log4j classes on the classpath