feat: add DEBUG-level logs for SPI extension loading in ExtensionLoader#16231
feat: add DEBUG-level logs for SPI extension loading in ExtensionLoader#16231uuuyuqi wants to merge 14 commits intoapache:3.3from
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16231 +/- ##
=========================================
Coverage 60.80% 60.80%
- Complexity 11759 11779 +20
=========================================
Files 1953 1953
Lines 89186 89238 +52
Branches 13454 13472 +18
=========================================
+ Hits 54227 54259 +32
- Misses 29391 29403 +12
- Partials 5568 5576 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is there anything else needed from my side before this PR can be merged? |
There was a problem hiding this comment.
Pull request overview
This PR adds DEBUG-level observability to org.apache.dubbo.common.extension.ExtensionLoader to make SPI extension discovery, class loading, and instance/wrapper creation diagnosable via logs (instead of requiring remote debugging tools).
Changes:
- Add DEBUG logs for extension class loading (start/finish with timing, names, adaptive/wrapper info) and per-class load/skip reasons.
- Add DEBUG log for extension instance creation, including whether wrapping occurred and which wrappers were applied.
- Add a unit test that attaches a Log4j2 appender to validate the new “extension instance loaded” DEBUG log output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java |
Adds DEBUG logging around SPI class loading, inactivity skips (@Activate(onClass=...)), and extension instance creation/wrapper application. |
dubbo-common/src/test/java/org/apache/dubbo/common/extension/ExtensionLoaderTest.java |
Adds a Log4j2-based log collector and a new test asserting the DEBUG log emitted when an extension instance is created. |
Comments suppressed due to low confidence (1)
dubbo-common/src/main/java/org/apache/dubbo/common/extension/ExtensionLoader.java:792
appliedWrapperClassesis allocated/populated on every wrapped extension creation, even when DEBUG logging is off. To keep overhead effectively zero when DEBUG is disabled, consider only tracking wrapper classes whenlogger.isDebugEnabled()and otherwise pass an empty list/null into the logging method.
List<Class<?>> appliedWrapperClasses = Collections.emptyList();
if (wrap) {
List<Class<?>> wrapperClassesList = new ArrayList<>();
if (cachedWrapperClasses != null) {
wrapperClassesList.addAll(cachedWrapperClasses);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LoggerConfig loggerConfig = new LoggerConfig(loggerName, org.apache.logging.log4j.Level.DEBUG, false); | ||
| loggerConfig.addAppender(appender, org.apache.logging.log4j.Level.DEBUG, null); | ||
| configuration.addLogger(loggerName, loggerConfig); | ||
| context.updateLoggers(); | ||
| return new LogCollector(context, configuration, loggerName, loggerConfig, appender); |
There was a problem hiding this comment.
LogCollector.attach installs a brand-new LoggerConfig for loggerName and close() removes it, which can clobber any pre-existing logger configuration for that name and cause cross-test side effects. Consider capturing the previous LoggerConfig (or using configuration.getLoggerConfig(loggerName) and only adding/removing an appender) and restoring the original level/additivity on close.
There was a problem hiding this comment.
Repairing this is not necessary because it is in the test class.
🤖 Generated with [Qoder][https://qoder.com] Change-Id: I9775997a1233f13a1faead1391ee01b743796c6d Co-developed-by: Qoder CLI <noreply@qoder.com>
8fc12fd to
fb8aa51
Compare
Refactor logging to only log extension instance loading and timing when debug is enabled.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void test_getExtension_logsDebugWhenExtensionCreated() { | ||
| try (ExtensionLoaderTestContext<WrappedExt> testContext = createExtensionLoaderTestContext(WrappedExt.class); | ||
| LogCollector logCollector = LogCollector.attach(ExtensionLoader.class)) { | ||
| WrappedExt impl1 = testContext.extensionLoader.getExtension("impl1"); |
There was a problem hiding this comment.
This test’s LogCollector only captures Log4j2 output, but ExtensionLoader logs through Dubbo’s LoggerFactory adapter, which is mutable global state and is set to non-log4j2 in other tests (e.g., LoggerFactoryTest, FailsafeErrorTypeAwareLoggerTest). That can make this test order-dependent/flaky because the DEBUG line may never reach Log4j2. Consider setting the logger adapter to log4j2 for the duration of this test (and restoring the previous adapter in close()/finally) before asserting on captured Log4j2 events.
| @@ -794,13 +795,15 @@ private T createExtension(String name, boolean wrap) { | |||
| } | |||
|
|
|||
| if (CollectionUtils.isNotEmpty(wrapperClassesList)) { | |||
| appliedWrapperClasses = new ArrayList<>(wrapperClassesList.size()); | |||
| for (Class<?> wrapperClass : wrapperClassesList) { | |||
| Wrapper wrapper = wrapperClass.getAnnotation(Wrapper.class); | |||
| boolean match = (wrapper == null) | |||
| || ((ArrayUtils.isEmpty(wrapper.matches()) | |||
| || ArrayUtils.contains(wrapper.matches(), name)) | |||
| && !ArrayUtils.contains(wrapper.mismatches(), name)); | |||
| if (match) { | |||
| appliedWrapperClasses.add(wrapperClass); | |||
There was a problem hiding this comment.
appliedWrapperClasses is allocated and populated on every wrapped extension creation, even when DEBUG logging is disabled. This adds extra allocations and per-wrapper work on a hot path and contradicts the stated “no performance impact when DEBUG is off”. Consider only tracking appliedWrapperClasses when logger.isDebugEnabled() (e.g., keep it null otherwise and guard add calls), or reuse the existing wrapperClassesList only when needed for logging.
|
|
||
| if (logger.isDebugEnabled()) { | ||
| Long costMillis = startNanos != -1 ? (System.nanoTime() - startNanos) / 1_000_000L : null; | ||
| Long costMillis = (System.nanoTime() - startNanos) / 1_000_000L; |
There was a problem hiding this comment.
At this point, startNanos might be null, for example, if the application dynamically modifies the log level at runtime. This is quite common in online troubleshooting scenarios. To be on the safe side, it's probably best to assign an initial value of null or -1.
| && !ArrayUtils.contains(wrapper.mismatches(), name)); | ||
| if (match) { | ||
| appliedWrapperClasses.add(wrapperClass); | ||
| if (logger.isDebugEnabled()) { |
There was a problem hiding this comment.
Dynamically modifying the log level might cause an NPE error. For performance optimization, consider adding a null check.
Removed unused Log4j imports and added preLoggerConfig to LogCollector.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…xtensionLoader.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added a method to retrieve the current logger adapter.
Removed unnecessary blank line in Javadoc comment.
Removed duplicate import statement for 'Prioritized'.
What is the purpose of the change?
Add DEBUG-level observability to
ExtensionLoaderfor diagnosing SPI extension loading issues without external tools.Motivation: In environments with many internal SPI extensions across different versions, along with custom class-isolation frameworks, dependency conflicts or misconfiguration can cause extensions to be silently skipped or loaded with unexpected wrapper chains. Currently the only way to observe this is via Arthas or remote debugging, which has high operational cost. With these DEBUG logs, users can simply enable DEBUG level for
org.apache.dubbo.common.extension.ExtensionLoaderto get full visibility into the SPI loading process.Changes:
@Activate(onClass=...)dependencies, with the specific missing class namesloadClassIfActivetofindMissingOnClassto return missing class names for diagnostic outputAll logs are guarded by
isDebugEnabled(), no performance impact in production.Fixes #16230
Checklist