-
-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[OSPP]Add more observability in apollo config client #74
base: main
Are you sure you want to change the base?
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
Warning Rate limit exceeded@Rawven has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 30 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ## Walkthrough
Recent updates to the Apollo framework enhance its monitoring capabilities through the introduction of new classes and interfaces for metrics collection, event handling, and structured logging. The changes include the implementation of various monitoring APIs, a new constants repository, and improvements in dependency injection, all aimed at improving observability and management of configuration metrics.
## Changes
| Files | Change Summary |
|-----------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------|
| `.../src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java` | Introduced `ConfigMonitorInitializer` class for initializing monitoring components within Apollo. |
| `.../src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java` | Enhanced `DefaultInjector` with new bindings for monitoring capabilities. |
| `.../src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java` | Improved monitoring and logging in `RemoteConfigRepository`, added timing for config loading. |
| `.../src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java` | Added `ApolloClientBootstrapArgsMonitorApi` interface for monitoring bootstrap arguments. |
| `.../src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java` | Introduced `ApolloClientExceptionMonitorApi` for tracking exceptions. |
| `.../src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java` | Added `ApolloClientNamespaceMonitorApi` for monitoring namespace metrics. |
| `.../src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java` | Introduced `ApolloClientThreadPoolMonitorApi` for monitoring thread pool metrics. |
| `.../src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java` | Created `ApolloClientMonitorConstant` for centralized constant management in monitoring. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant ConfigMonitorInitializer
participant MetricsExporter
participant Logger
Client->>ConfigMonitorInitializer: Initialize Monitoring
ConfigMonitorInitializer->>MetricsExporter: Setup Metrics Collectors
MetricsExporter-->>ConfigMonitorInitializer: Confirm Setup
ConfigMonitorInitializer->>Logger: Log Initialization Events
Logger-->>ConfigMonitorInitializer: Confirm Log
ConfigMonitorInitializer-->>Client: Monitoring Initialized
|
df5e94d
to
7de469e
Compare
@Anilople PTAL |
b0a1990
to
1ba4e6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 25
Outside diff range, codebase verification and nitpick comments (17)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1)
19-23
: Class-level Javadoc is clear but could be more descriptive.The Javadoc for the class
MonitorConstant
is clear but could benefit from a more detailed description of the class's purpose.- * metrics constant + * This class holds constants used for metrics in the Apollo monitoring system.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java (1)
22-24
: Class-level Javadoc is minimal but acceptable.The Javadoc for the interface
ApolloExceptionMonitorApi
is minimal. It would be beneficial to add a more detailed description of the interface's purpose.- * @author Rawven + * This MXBean interface defines methods for monitoring exceptions in the Apollo client.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollectorManager.java (1)
22-24
: Class-level Javadoc is minimal but acceptable.The Javadoc for the interface
MetricsCollectorManager
is minimal. It would be beneficial to add a more detailed description of the interface's purpose.- * @author Rawven + * This interface defines methods for managing metrics collectors in the Apollo monitoring system.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1)
22-32
: Consider adding Javadoc comments for interface methods.Adding Javadoc comments for each method will improve the readability and maintainability of the code by providing clear descriptions of the methods' purposes and expected behavior.
/** * Interface for monitoring various aspects of the Apollo client. */ public interface ConfigMonitor { /** * Gets the thread pool monitor API. * * @return the thread pool monitor API */ ApolloThreadPoolMonitorApi getThreadPoolMonitorApi(); /** * Gets the exception monitor API. * * @return the exception monitor API */ ApolloExceptionMonitorApi getExceptionMonitorApi(); /** * Gets the namespace monitor API. * * @return the namespace monitor API */ ApolloNamespaceMonitorApi getNamespaceMonitorApi(); /** * Gets the running parameters monitor API. * * @return the running parameters monitor API */ ApolloRunningParamsMonitorApi getRunningParamsMonitorApi(); /** * Gets data formatted according to the current monitoring system. * * @return data in the current monitoring system format */ String getDataWithCurrentMonitoringSystemFormat(); }apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullExceptionMonitorApi.java (1)
23-33
: Consider adding Javadoc comments for class and methods.Adding Javadoc comments will improve the readability and maintainability of the code by providing clear descriptions of the class's purpose and the methods' behavior.
/** * A null implementation of the {@link ApolloExceptionMonitorApi} that returns default values. */ public class NullExceptionMonitorApi implements ApolloExceptionMonitorApi { /** * Returns the number of exceptions. * * @return the number of exceptions, always 0 */ @Override public Integer getExceptionNum() { return 0; } /** * Returns the details of exceptions. * * @return an empty list, indicating no exceptions */ @Override public List<String> getExceptionDetails() { return Collections.emptyList(); } }apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java (1)
26-47
: Consider adding Javadoc comments for class and methods.Adding Javadoc comments will improve the readability and maintainability of the code by providing clear descriptions of the class's purpose and the methods' behavior.
/** * A model representing metrics with tags, name, and type. */ public class MetricsModel { protected final Map<String, String> tags = new HashMap<>(); protected String name; protected MeterType type; /** * Gets the name of the metric. * * @return the name of the metric prefixed with "Apollo_Client_" */ public String getName() { return "Apollo_Client_" + name; } /** * Sets the name of the metric. * * @param name the name of the metric */ public void setName(String name) { this.name = name; } /** * Gets the type of the metric. * * @return the type of the metric */ public MeterType getType() { return type; } /** * Gets the tags associated with the metric. * * @return the tags associated with the metric */ public Map<String, String> getTags() { return tags; } }apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java (3)
28-28
: Consider adding more detailed documentation.While the interface is well-defined, adding more detailed documentation for each method would improve readability and maintainability.
33-33
: Clarify the purpose of theinit
method parameters.The
init
method lacks detailed documentation on the parameters, especiallycollectPeriod
. Consider adding descriptions for better clarity.
40-40
: Clarify the purpose of theisSupport
method parameter.The
isSupport
method lacks detailed documentation on theform
parameter. Consider adding a description for better clarity.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullRunningParamsMonitorApi.java (1)
21-21
: ClassNullRunningParamsMonitorApi
does not fully implementApolloRunningParamsMonitorApi
.The following methods from
ApolloRunningParamsMonitorApi
are not implemented inNullRunningParamsMonitorApi
:
getAccessKeySecret
getAutoUpdateInjectedSpringProperties
getBootstrapEnabled
Please ensure that all methods from the interface are implemented.
Analysis chain
Class
NullRunningParamsMonitorApi
implementsApolloRunningParamsMonitorApi
.Ensure that
ApolloRunningParamsMonitorApi
provides necessary functionality and is correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and implementation of `ApolloRunningParamsMonitorApi`. # Test: Search for the `ApolloRunningParamsMonitorApi` interface definition. Expect: Correct interface definition and implementation. rg --type java -A 10 $'interface ApolloRunningParamsMonitorApi'Length of output: 1426
Script:
#!/bin/bash # Description: Verify the implementation of methods from ApolloRunningParamsMonitorApi in NullRunningParamsMonitorApi. # Test: Search for the method implementations in NullRunningParamsMonitorApi. rg --type java -A 10 'class NullRunningParamsMonitorApi' -g '*.java' | grep -E 'getStartupParams|getConfigServiceUrl|getAccessKeySecret|getAutoUpdateInjectedSpringProperties|getBootstrapEnabled'Length of output: 493
Script:
#!/bin/bash # Description: Verify the implementation of missing methods from ApolloRunningParamsMonitorApi in NullRunningParamsMonitorApi. # Test: Search for the specific method implementations in NullRunningParamsMonitorApi. rg --type java -A 10 'class NullRunningParamsMonitorApi' -g '*.java' | grep -E 'getAccessKeySecret|getAutoUpdateInjectedSpringProperties|getBootstrapEnabled'Length of output: 159
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java (2)
141-145
: Method implementation is a placeholder.The
logEvent(String type, String name, String status, String nameValuePairs)
method is currently a placeholder and does not log events.Consider implementing the event logging functionality or adding a TODO comment to indicate future implementation.
147-150
: Method returns null.The
newTransaction(String type, String name)
method currently returns null.Consider implementing the transaction creation functionality or adding a TODO comment to indicate future implementation.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (3)
65-77
: Consider adding comments for better readability.Adding comments explaining the purpose of each collector can improve the readability of the code.
DefaultConfigManager configManager = (DefaultConfigManager) ApolloInjector.getInstance(ConfigManager.class); // Initialize exception collector DefaultApolloExceptionCollector exceptionCollector = new DefaultApolloExceptionCollector(); // Initialize thread pool collector DefaultApolloThreadPoolCollector threadPoolCollector = new DefaultApolloThreadPoolCollector( RemoteConfigRepository.m_executorService, AbstractConfig.m_executorService, AbstractConfigFile.m_executorService); // Initialize namespace collector DefaultApolloNamespaceCollector namespaceCollector = new DefaultApolloNamespaceCollector( configManager.m_configs, configManager.m_configLocks, configManager.m_configFiles, configManager.m_configFileLocks); // Initialize running parameters collector DefaultApolloRunningParamsCollector startupCollector = new DefaultApolloRunningParamsCollector(m_configUtil);
84-91
: Consider adding comments for better readability.Adding comments explaining the initialization process can improve the readability of the code.
DefaultConfigMonitor defaultConfigMonitor = (DefaultConfigMonitor) ApolloInjector.getInstance(ConfigMonitor.class); // Retrieve specific collectors DefaultApolloExceptionCollector exceptionCollector = (DefaultApolloExceptionCollector) collectors.get(0); DefaultApolloNamespaceCollector namespaceCollector = (DefaultApolloNamespaceCollector) collectors.get(1); DefaultApolloThreadPoolCollector threadPoolCollector = (DefaultApolloThreadPoolCollector) collectors.get(2); DefaultApolloRunningParamsCollector startupCollector = (DefaultApolloRunningParamsCollector) collectors.get(3); // Initialize the config monitor with the collectors and metrics exporter defaultConfigMonitor.init(namespaceCollector, threadPoolCollector, exceptionCollector, startupCollector, metricsExporter);
93-112
: Consider adding comments for better readability.Adding comments explaining the purpose of each producer can improve the readability of the code.
// Prioritize loading user-defined producers from SPI List<MessageProducer> producers = ServiceBootstrap.loadAllOrdered(MessageProducer.class); // Add the producer that comes with the client if monitoring is enabled if (m_configUtil.isClientMonitorEnabled()) { producers.add(new MonitorMessageProducer()); } // Add CatMessageProducer if the class is present if (ClassLoaderUtil.isClassPresent(CatNames.CAT_CLASS)) { producers.add(new CatMessageProducer()); } // Add a default producer if no other producers are available if (producers.isEmpty()) { producers.add(new NullMessageProducer()); } return new MessageProducerComposite(producers);apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloRunningParamsCollector.java (2)
58-85
: Consider adding comments for better readability.Adding comments explaining the purpose of each parameter can improve the readability of the code.
public DefaultApolloRunningParamsCollector(ConfigUtil configUtil) { super(RUNNING_PARAMS, RUNNING_PARAMS); // Initialize map with various configuration parameters map.put(APOLLO_ACCESS_KEY_SECRET, configUtil.getAccessKeySecret()); map.put(APOLLO_AUTO_UPDATE_INJECTED_SPRING_PROPERTIES, configUtil.isAutoUpdateInjectedSpringPropertiesEnabled()); map.put(APOLLO_BOOTSTRAP_ENABLED, Boolean.parseBoolean(System.getProperty(APOLLO_BOOTSTRAP_ENABLED))); map.put(APOLLO_BOOTSTRAP_NAMESPACES, System.getProperty(APOLLO_BOOTSTRAP_NAMESPACES)); map.put(APOLLO_BOOTSTRAP_EAGER_LOAD_ENABLED, Boolean.parseBoolean(System.getProperty(APOLLO_BOOTSTRAP_EAGER_LOAD_ENABLED))); map.put(APOLLO_OVERRIDE_SYSTEM_PROPERTIES, configUtil.isOverrideSystemProperties()); map.put(APOLLO_CACHE_DIR, configUtil.getDefaultLocalCacheDir()); map.put(APOLLO_CLUSTER, configUtil.getCluster()); map.put(APOLLO_CONFIG_SERVICE, System.getProperty(APOLLO_CONFIG_SERVICE)); map.put(APOLLO_CLIENT_MONITOR_EXTERNAL_TYPE, configUtil.getMonitorExternalType()); map.put(APOLLO_CLIENT_MONITOR_ENABLED, configUtil.isClientMonitorEnabled()); map.put(APOLLO_CLIENT_MONITOR_EXTERNAL_EXPORT_PERIOD, configUtil.getMonitorExternalExportPeriod()); map.put(APOLLO_META, configUtil.getMetaServerDomainName()); map.put(APOLLO_PROPERTY_NAMES_CACHE_ENABLE, configUtil.isPropertyNamesCacheEnabled()); map.put(APOLLO_PROPERTY_ORDER_ENABLE, configUtil.isPropertiesOrderEnabled()); map.put(APOLLO_CLIENT_MONITOR_JMX_ENABLED, configUtil.isClientMonitorJmxEnabled()); map.put(APP_ID, configUtil.getAppId()); map.put(ENV, configUtil.getApolloEnv()); map.put(VERSION, Apollo.VERSION); }
92-106
: Consider adding comments for better readability.Adding comments explaining the purpose of each case can improve the readability of the code.
@Override public void collect0(MetricsEvent event) { switch (event.getName()) { case VERSION: // Update version in the map map.put(VERSION, event.getAttachmentValue(VERSION)); break; case META_FRESH: // Update meta fresh time in the map map.put(META_FRESH, event.getAttachmentValue(META_FRESH)); break; case CONFIG_SERVICE_URL: // Update config service URL in the map map.put(CONFIG_SERVICE_URL, event.getAttachmentValue(CONFIG_SERVICE_URL)); break; default: break; } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (55)
- apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloRunningParamsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullRunningParamsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractMetricsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloRunningParamsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/internals/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEventPusher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMessageProducerManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/MeterType.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializer.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (8 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ExceptionUtil.java (1 hunks)
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1 hunks)
- apollo-client/src/main/resources/META-INF/services/com.ctrip.framework.apollo.tracer.spi.MessageProducerManager (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractApolloMetricsCollectorTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
Files not processed due to max files limit (9)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/internals/DefaultMessageProducerManager.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/internals/cat/CatTransaction.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/spi/MessageProducer.java
- apollo-plugin/apollo-plugin-client-prometheus/pom.xml
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/plugin/prometheus/PrometheusMetricExporter.java
- apollo-plugin/apollo-plugin-client-prometheus/src/main/resources/META-INF/services/com.ctrip.framework.apollo.monitor.internal.exporter.MetricsExporter
- apollo-plugin/pom.xml
Files skipped from review due to trivial changes (6)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ExceptionUtil.java
Additional comments not posted (177)
apollo-client/src/main/resources/META-INF/services/com.ctrip.framework.apollo.tracer.spi.MessageProducerManager (1)
1-1
: LGTM!The service provider configuration looks correct.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/MeterType.java (2)
1-16
: LGTM!The license header follows the standard Apache License 2.0 format.
17-25
: LGTM!The enum definition is correct and follows best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporterFactory.java (2)
1-16
: LGTM!The license header follows the standard Apache License 2.0 format.
17-28
: LGTM!The interface definition is correct and follows best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (3)
1-16
: Correct and compliant license header.The license header is correctly included and compliant with the Apache License 2.0 requirements.
17-17
: Package declaration is correct.The package declaration is consistent with the project's structure.
24-29
: Constants are well-defined.The constants are well-defined and named appropriately for their intended use.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java (4)
1-16
: Correct and compliant license header.The license header is correctly included and compliant with the Apache License 2.0 requirements.
17-17
: Package declaration is correct.The package declaration is consistent with the project's structure.
19-21
: Imports are appropriate.The imports are appropriate for the functionality provided by this interface.
25-37
: Interface and methods are well-defined.The interface and its methods are well-defined and follow standard conventions for MXBeans. The method names are descriptive and appropriate for their intended use.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollectorManager.java (4)
1-16
: Correct and compliant license header.The license header is correctly included and compliant with the Apache License 2.0 requirements.
17-17
: Package declaration is correct.The package declaration is consistent with the project's structure.
19-21
: Imports are appropriate.The imports are appropriate for the functionality provided by this interface.
25-36
: Interface and methods are well-defined.The interface and its methods are well-defined and follow standard conventions. The method names are descriptive and appropriate for their intended use.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultMetricsCollectorManager.java (4)
1-17
: LGTM!The file header and package declaration are correct.
19-22
: LGTM!The imports are correct and necessary for the implementation.
23-28
: LGTM!The class declaration and fields are correct.
30-40
: LGTM!The methods are correct and follow best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMessageProducerManager.java (4)
1-17
: LGTM!The file header and package declaration are correct.
19-22
: LGTM!The imports are correct and necessary for the implementation.
23-28
: LGTM!The class declaration and fields are correct.
30-42
: LGTM!The methods are correct and follow best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollector.java (3)
1-17
: LGTM!The file header and package declaration are correct.
19-22
: LGTM!The imports are correct and necessary for the interface.
23-52
: LGTM!The interface declaration and methods are correct and follow best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloNamespaceMonitorApi.java (11)
28-28
: LGTM!The method
getNamespaceReleaseKey
is correctly defined.
30-30
: LGTM!The method
getNamespaceUsageCount
is correctly defined.
32-32
: LGTM!The method
getNamespaceLatestUpdateTime
is correctly defined.
34-34
: LGTM!The method
getNamespaceFirstLoadSpend
is correctly defined.
36-36
: LGTM!The method
getNamespace404
is correctly defined.
38-38
: LGTM!The method
getNamespaceTimeout
is correctly defined.
40-40
: LGTM!The method
getNamespaceItemName
is correctly defined.
42-42
: LGTM!The method
getAllNamespaceReleaseKey
is correctly defined.
44-44
: LGTM!The method
getAllNamespaceUsageCount
is correctly defined.
46-46
: LGTM!The method
getAllNamespacesLatestUpdateTime
is correctly defined.
48-52
: LGTM!The methods
getAllUsedNamespaceName
,getAllNamespaceFirstLoadSpend
, andgetAllNamespaceItemName
are correctly defined.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEventPusher.java (2)
29-31
: LGTM!The static initialization block is correctly defined.
33-42
: LGTM!The method
push
is correctly defined.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloRunningParamsMonitorApi.java (1)
27-69
: LGTM!The methods in
ApolloRunningParamsMonitorApi
are correctly defined.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java (1)
1-16
: LGTM!The class-level comments and annotations are clear and follow the standard licensing format.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java (1)
1-16
: LGTM!The class-level comments and annotations are clear and follow the standard licensing format.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1)
1-16
: LGTM!The class-level comments and annotations are clear and follow the standard licensing format.
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (2)
46-51
: LGTM!The test method correctly verifies that
getMetricsReporter
returns null when no supported reporter is available.
53-59
: LGTM!The test method correctly verifies that
getMetricsReporter
returns null when the form is null.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractMetricsCollector.java (6)
43-46
: LGTM!The constructor correctly initializes the name and tags for the metrics collector.
49-51
: LGTM!The
name
method correctly returns the name of the metrics collector.
54-56
: LGTM!The
isSupport
method correctly checks if the collector supports the given metrics event based on its tag.
59-62
: LGTM!The
collect
method correctly collects the given metrics event and sets theisUpdated
flag to true.
65-67
: LGTM!The
isSamplesUpdated
method correctly returns the value of theisUpdated
flag and resets it to false.
70-74
: LGTM!The
export
method correctly exports the collected metrics samples.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2)
19-19
: Good practice: Use of a constant instead of a hardcoded string.The use of
APOLLO_CONFIG_EXCEPTION
constant improves maintainability and reduces the risk of typos.
46-46
: Improved maintainability: Use of constant inTracer.logEvent
.Replacing the hardcoded string with
APOLLO_CONFIG_EXCEPTION
enhances code clarity and consistency.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloThreadPoolMonitorApi.java (1)
1-89
: New interface for thread pool monitoring looks good.The
ApolloThreadPoolMonitorApi
interface provides comprehensive methods for monitoring thread pool metrics, enhancing observability.apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractApolloMetricsCollectorTest.java (1)
1-90
: New test class for metrics collector looks good.The
AbstractApolloMetricsCollectorTest
class is well-structured and covers essential functionalities of theAbstractMetricsCollector
class using JUnit and Mockito.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (6)
33-38
: Constructor initializes GaugeModel with provided parameters.Ensure that the parameters are correctly passed and initialized.
40-42
: Builder pattern implementation forGaugeModel
.The builder pattern is correctly implemented.
44-50
: Getter and setter methods forvalue
andapply
.The methods are correctly implemented.
60-61
: MethodgetApplyValue
returns the applied value.The method is correctly implemented.
64-96
: NestedGaugeBuilder
class for buildingGaugeModel
instances.The builder class is correctly implemented.
24-24
: ClassGaugeModel
extendsMetricsModel
.Ensure that
MetricsModel
provides necessary functionality and is correctly extended.Verification successful
Class
GaugeModel
extendsMetricsModel
.The
MetricsModel
class provides the necessary functionality for theGaugeModel
class, including protected instance variables and methods for managing metrics.
MetricsModel
has protected instance variables:tags
,name
, andtype
.MetricsModel
includesgetName
andsetName
methods.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and implementation of `MetricsModel`. # Test: Search for the `MetricsModel` class definition. Expect: Correct class definition and implementation. rg --type java -A 10 $'class MetricsModel'Length of output: 1350
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullRunningParamsMonitorApi.java (22)
23-25
: MethodgetStartupParams
returns an empty string.The method is correctly implemented.
28-30
: MethodgetConfigServiceUrl
returns an empty string.The method is correctly implemented.
33-35
: MethodgetAccessKeySecret
returns an empty string.The method is correctly implemented.
38-40
: MethodgetAutoUpdateInjectedSpringProperties
returns null.The method is correctly implemented.
43-45
: MethodgetBootstrapEnabled
returns null.The method is correctly implemented.
48-50
: MethodgetBootstrapNamespaces
returns an empty string.The method is correctly implemented.
53-55
: MethodgetBootstrapEagerLoadEnabled
returns null.The method is correctly implemented.
58-60
: MethodgetOverrideSystemProperties
returns null.The method is correctly implemented.
63-65
: MethodgetCacheDir
returns an empty string.The method is correctly implemented.
68-70
: MethodgetCluster
returns an empty string.The method is correctly implemented.
73-75
: MethodgetConfigService
returns an empty string.The method is correctly implemented.
78-80
: MethodgetClientMonitorEnabled
returns null.The method is correctly implemented.
83-85
: MethodgetClientMonitorJmxEnabled
returns null.The method is correctly implemented.
88-90
: MethodgetClientMonitorExternalForm
returns an empty string.The method is correctly implemented.
93-95
: MethodgetClientMonitorExternalExportPeriod
returns zero.The method is correctly implemented.
98-100
: MethodgetMeta
returns an empty string.The method is correctly implemented.
103-105
: MethodgetMetaLatestFreshTime
returns an empty string.The method is correctly implemented.
108-110
: MethodgetPropertyNamesCacheEnable
returns null.The method is correctly implemented.
113-115
: MethodgetPropertyOrderEnable
returns null.The method is correctly implemented.
118-120
: MethodgetVersion
returns an empty string.The method is correctly implemented.
123-125
: MethodgetEnv
returns an empty string.The method is correctly implemented.
128-130
: MethodgetAppId
returns an empty string.The method is correctly implemented.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (8)
39-42
: MethodgetThreadPoolMonitorApi
returns the thread pool monitor API.The method is correctly implemented.
44-47
: MethodgetExceptionMonitorApi
returns the exception monitor API.The method is correctly implemented.
49-52
: MethodgetNamespaceMonitorApi
returns the namespace monitor API.The method is correctly implemented.
54-57
: MethodgetRunningParamsMonitorApi
returns the running params monitor API.The method is correctly implemented.
59-65
: MethodgetDataWithCurrentMonitoringSystemFormat
returns the monitoring system data.The method is correctly implemented.
67-77
: Methodinit
initializes the monitoring APIs and metrics exporter.The method is correctly implemented.
33-37
: Instance variables for monitoring APIs and metrics exporter.Ensure that the default implementations (
NullThreadPoolMonitorApi
,NullExceptionMonitorApi
,NullNamespaceMonitorApi
, andNullRunningParamsMonitorApi
) are correctly implemented and provide necessary functionality.Verification successful
Default implementations of monitoring APIs are correctly implemented.
The
NullThreadPoolMonitorApi
,NullExceptionMonitorApi
,NullNamespaceMonitorApi
, andNullRunningParamsMonitorApi
classes are correctly defined and provide the expected default functionality.
NullThreadPoolMonitorApi
returns0
for thread pool metrics.NullExceptionMonitorApi
returns0
and an empty list for exception metrics.NullNamespaceMonitorApi
returns empty strings and0
for namespace metrics.NullRunningParamsMonitorApi
returns empty strings for running parameters.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and implementation of default monitoring APIs. # Test: Search for the `NullThreadPoolMonitorApi` class definition. Expect: Correct class definition and implementation. rg --type java -A 10 $'class NullThreadPoolMonitorApi' # Test: Search for the `NullExceptionMonitorApi` class definition. Expect: Correct class definition and implementation. rg --type java -A 10 $'class NullExceptionMonitorApi' # Test: Search for the `NullNamespaceMonitorApi` class definition. Expect: Correct class definition and implementation. rg --type java -A 10 $'class NullNamespaceMonitorApi' # Test: Search for the `NullRunningParamsMonitorApi` class definition. Expect: Correct class definition and implementation. rg --type java -A 10 $'class NullRunningParamsMonitorApi'Length of output: 5724
31-31
: ClassDefaultConfigMonitor
implementsConfigMonitor
.Ensure that
ConfigMonitor
provides necessary functionality and is correctly implemented.Verification successful
Class
DefaultConfigMonitor
correctly implements theConfigMonitor
interface.The
DefaultConfigMonitor
class includes implementations for all the methods defined in theConfigMonitor
interface:
getThreadPoolMonitorApi
getExceptionMonitorApi
getNamespaceMonitorApi
getRunningParamsMonitorApi
getDataWithCurrentMonitoringSystemFormat
No issues were found with the implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and implementation of `ConfigMonitor`. # Test: Search for the `ConfigMonitor` interface definition. Expect: Correct interface definition and implementation. rg --type java -A 10 $'interface ConfigMonitor'Length of output: 1311
Script:
#!/bin/bash # Description: Verify the implementation of `ConfigMonitor` methods in `DefaultConfigMonitor`. # Test: Search for the implementation of interface methods in `DefaultConfigMonitor`. rg --type java -A 10 'class DefaultConfigMonitor' | rg 'getThreadPoolMonitorApi|getExceptionMonitorApi|getNamespaceMonitorApi|getRunningParamsMonitorApi|getDataWithCurrentMonitoringSystemFormat'Length of output: 358
Script:
#!/bin/bash # Description: Verify the implementation of remaining `ConfigMonitor` methods in `DefaultConfigMonitor`. # Test: Search for the implementation of the remaining interface methods in `DefaultConfigMonitor`. rg --type java -A 10 'class DefaultConfigMonitor' | rg 'getExceptionMonitorApi|getNamespaceMonitorApi|getRunningParamsMonitorApi|getDataWithCurrentMonitoringSystemFormat'Length of output: 172
Script:
#!/bin/bash # Description: Verify the implementation of remaining `ConfigMonitor` methods in `DefaultConfigMonitor`. # Test: Search for the implementation of the remaining interface methods in `DefaultConfigMonitor.java`. rg 'getExceptionMonitorApi|getNamespaceMonitorApi|getRunningParamsMonitorApi|getDataWithCurrentMonitoringSystemFormat' apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.javaLength of output: 473
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloExceptionCollector.java (2)
47-49
: Constructor Initialization Looks GoodThe constructor correctly initializes the superclass with predefined constants.
51-54
: MethodgetExceptionNum
Looks GoodThe method correctly returns the current value of
exceptionNum
.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (4)
45-47
: Constructor Initialization Looks GoodThe constructor correctly initializes the
producers
list.
50-53
: MethodlogError(Throwable cause)
Looks GoodThe method correctly logs errors using each producer in the list.
55-58
: MethodlogError(String message, Throwable cause)
Looks GoodThe method correctly logs errors with a message using each producer in the list.
60-63
: MethodlogEvent(String type, String name)
Looks GoodThe method correctly logs events using each producer in the list.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (4)
19-20
: New Import Statements Look GoodThe new import statements for monitoring functionality are necessary and correctly added.
Also applies to: 26-27
38-41
: Access Modifier Changes Look GoodThe changes enhance the visibility of these variables, allowing subclasses to access them directly.
66-68
: New Monitoring Functionality ingetConfig
Method Looks GoodThe new functionality integrates monitoring capabilities into the configuration retrieval process, which is beneficial for tracking usage patterns.
74-75
: Formatting Change ingetConfigFile
Method Looks GoodThe formatting change improves readability without altering the logic.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/internals/DefaultMetricsExporterFactory.java (1)
41-43
: LGTM! Constructor is correctly initializing the configuration utility.The constructor initializes
m_configUtil
usingApolloInjector
.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java (4)
45-48
: LGTM! Static initializer block is correctly setting up the scheduled executor service.The static initializer block initializes
m_executorService
with a scheduled thread pool.
59-59
: LGTM! Abstract method placeholder for subclass implementations.The method is abstract and meant to be implemented by subclasses.
80-96
: LGTM! Method handles different sample types appropriately.The method registers different types of metrics samples.
98-106
: LGTM! Method correctly handles null or empty tags.The method extracts tags from a
MetricsModel
and returns them as a 2D array.apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloMetricsExporterTest.java (4)
79-87
: LGTM! Test method correctly verifies the initialization process.The test method verifies the initialization of the metrics exporter.
88-92
: LGTM! Test method correctly verifies the support check.The test method verifies the
isSupport
method of the metrics exporter.
94-107
: LGTM! Test method correctly verifies the metrics data update process.The test method verifies the
updateMetricsData
method of the metrics exporter.
109-122
: LGTM! Test method correctly verifies the tag extraction process.The test method verifies the
getTags
method of the metrics exporter.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullThreadPoolMonitorApi.java (4)
1-16
: File header and package declaration look good.The file header contains the appropriate license information, and the package declaration is correct.
21-71
: Correct implementation ofApolloThreadPoolMonitorApi
methods forRemoteConfigRepository
.The methods correctly return default values as expected for a null implementation.
73-121
: Correct implementation ofApolloThreadPoolMonitorApi
methods forAbstractConfig
.The methods correctly return default values as expected for a null implementation.
123-171
: Correct implementation ofApolloThreadPoolMonitorApi
methods forAbstractConfigFile
.The methods correctly return default values as expected for a null implementation.
apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java (5)
23-24
: New imports forConfigMonitorInitializer
andConfigMonitor
.The new imports are necessary for the added functionality.
35-35
: New volatile member variablem_configMonitor
.The new member variable is correctly declared as volatile for thread-safe access.
39-49
: Thread-safe initialization ofm_configMonitor
ingetMonitor
method.The method correctly implements double-checked locking for thread-safe initialization of
m_configMonitor
.
50-56
: Initialization ofConfigMonitorInitializer
ingetManager
method.The
ConfigMonitorInitializer.initialize()
is correctly called after them_configManager
is instantiated.
98-100
: New methodgetConfigMonitor
for external access toConfigMonitor
.The method correctly provides external access to the
ConfigMonitor
instance.apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (4)
66-72
: New configuration entryapollo.client.monitor.enabled
.The entry is correctly defined with appropriate type, source, description, and default value.
73-79
: New configuration entryapollo.client.monitor.jmx.enabled
.The entry is correctly defined with appropriate type, source, description, and default value.
80-86
: New configuration entryapollo.client.monitor.external.type
.The entry is correctly defined with appropriate type, source, description, and default value.
87-94
: New configuration entryapollo.client.monitor.external.export-period
.The entry is correctly defined with appropriate type, source, description, and default value.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (2)
20-25
: Imports are necessary and relevant.The new import statements for monitoring and metrics management classes are necessary and relevant to the changes made in the file.
115-117
: Bindings are correctly configured.The new bindings for
ConfigMonitor
,MetricsCollectorManager
, andMetricsExporterFactory
are correctly configured and follow best practices.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java (5)
52-58
: Method correctly logs error with throwable.The
logError(Throwable cause)
method correctly logs an error with the provided throwable.
60-65
: Method correctly logs error with message and throwable.The
logError(String message, Throwable cause)
method correctly logs an error with the provided message and throwable.
67-74
: Method correctly logs events based on type and name.The
logEvent(String type, String name)
method correctly logs events based on the provided type and name.
76-126
: Method correctly handles tagged events.The
handleTaggedEvent(String type, String name)
method correctly handles tagged events based on the provided type and name.
129-139
: Method correctly handles client config events.The
handleClientConfigEvent(String type, String name)
method correctly handles client config events based on the provided type and name.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (2)
49-49
: Visibility change is appropriate for extensibility.The visibility change of
m_executorService
fromprivate
toprotected
is appropriate and follows best practices for extensibility.
118-118
: Modification improves readability and maintainability.The modification to use a static import of
APOLLO_CLIENT_CONFIGCHANGES
in theTracer.logEvent
call improves code readability and maintainability.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (2)
61-63
: LGTM!The method correctly retrieves an instance of
MetricsCollectorManager
.
79-82
: LGTM!The method correctly retrieves an instance of
MetricsExporterFactory
and gets aMetricsExporter
.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloRunningParamsCollector.java (5)
87-90
: LGTM!The method correctly returns the name of the collector.
109-112
: LGTM!The method correctly returns false.
114-116
: LGTM! Consider adding a TODO comment if future implementation is planned.The method is currently empty and does not perform any actions.
119-121
: LGTM!The method correctly retrieves a value from the map based on the key.
124-126
: LGTM!The method correctly retrieves the config service URL from the map.
apollo-client/src/main/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializer.java (3)
96-100
: LGTM!The added constants align with the PR objectives to enhance observability.
139-139
: LGTM!The addition ensures that the
APOLLO_BOOTSTRAP_NAMESPACES
property is explicitly defined in the system environment.
205-212
: LGTM!The additions ensure that the
APOLLO_BOOTSTRAP_EAGER_LOAD_ENABLED
andAPOLLO_BOOTSTRAP_ENABLED
properties are explicitly defined in the system environment.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloThreadPoolCollector.java (5)
64-71
: LGTM!The method correctly exports thread pool metrics for each executor.
74-77
: LGTM!The method correctly indicates that samples are always updated.
103-153
: LGTM!The methods correctly return various metrics for the
remoteConfigRepositoryExecutorService
.
155-204
: LGTM!The methods correctly return various metrics for the
abstractConfigExecutorService
.
207-255
: LGTM!The methods correctly return various metrics for the
abstractConfigFileExecutorService
.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (1)
19-20
: LGTM!The change improves maintainability by reducing the risk of typos in the event name and centralizing the definition of the event identifier.
Also applies to: 241-241
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java (5)
72-81
: LGTM!The constructor correctly initializes the fields and calls the superclass constructor.
133-143
: LGTM!The method correctly updates counter samples.
145-158
: LGTM!The method correctly updates gauge samples.
161-244
: LGTM!The methods correctly return various metrics for namespaces.
246-294
: LGTM!The class correctly encapsulates metrics for a namespace.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (3)
218-218
: Improved error logging.The use of
Tracer.logEvent(APOLLO_CONFIG_EXCEPTION, ExceptionUtil.getDetailMessage(ex));
improves the consistency and clarity of error logging.
221-224
: Enhanced monitoring for SocketTimeoutException.The new block for handling
SocketTimeoutException
improves monitoring by logging a metrics event.
19-20
: Ensure the imported constants are used correctly.The new import statements for
NAMESPACE
andAPOLLO_CONFIG_EXCEPTION
should be verified to ensure they are used correctly within the class.Verification successful
The imported constants are used correctly.
The constants
NAMESPACE
andAPOLLO_CONFIG_EXCEPTION
are utilized within theRemoteConfigLongPollService
class, confirming their necessity and correct usage.
NAMESPACE
is used in logging and monitoring activities.APOLLO_CONFIG_EXCEPTION
is used for tracing configuration exceptions.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imported constants. # Test: Search for usage of the imported constants. Expect: Only occurrences of the new constants. rg --type java -A 5 $'NAMESPACE|APOLLO_CONFIG_EXCEPTION'Length of output: 244113
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9)
87-87
: Visibility change form_executorService
.The visibility of
m_executorService
has been changed fromprivate
toprotected
, which allows subclasses to access it. Ensure that this change is necessary and does not introduce any security or encapsulation issues.
125-130
: Enhanced monitoring for configuration loading.The new timing mechanism and metrics event logging improve monitoring of the configuration loading process.
152-155
: Enhanced logging for periodic refresh.The use of
Tracer.logEvent
with constants improves the consistency and clarity of logging.
179-179
: Enhanced logging for configuration events.The use of
Tracer.logEvent
with constants improves the consistency and clarity of logging.
210-210
: Improved logging for configuration metadata.The use of
Tracer.logEvent
with constants improves the consistency and clarity of logging.
281-282
: Enhanced monitoring for missing namespaces.The new metrics event logging improves monitoring when a namespace is not found.
284-284
: Improved error logging.The use of
Tracer.logEvent(APOLLO_CONFIG_EXCEPTION, ExceptionUtil.getDetailMessage(statusCodeException));
improves the consistency and clarity of error logging.
291-291
: Improved error logging.The use of
Tracer.logEvent(APOLLO_CONFIG_EXCEPTION, ExceptionUtil.getDetailMessage(ex));
improves the consistency and clarity of error logging.
19-26
: Ensure the imported constants are used correctly.The new import statements for various monitoring and logging constants should be verified to ensure they are used correctly within the class.
Verification successful
The imported constants are used correctly.
The new import statements for various monitoring and logging constants are indeed utilized within the class and other parts of the codebase.
NAMESPACE
andTIMESTAMP
are used inRemoteConfigRepository.java
and other files.NAMESPACE_MONITOR
is used inRemoteConfigRepository.java
and other files.APOLLO_CLIENT_CONFIGS
,APOLLO_CLIENT_CONFIGMETA
,APOLLO_CLIENT_VERSION
,APOLLO_CONFIGSERVICE
, andAPOLLO_CONFIG_EXCEPTION
are used inRemoteConfigRepository.java
and other files.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imported constants. # Test: Search for usage of the imported constants. Expect: Only occurrences of the new constants. rg --type java -A 5 $'NAMESPACE|TIMESTAMP|NAMESPACE_MONITOR|APOLLO_CLIENT_CONFIGS|APOLLO_CLIENT_CONFIGMETA|APOLLO_CLIENT_VERSION|APOLLO_CONFIGSERVICE|APOLLO_CONFIG_EXCEPTION'Length of output: 258295
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (10)
41-41
: Initialization moved to constructor.The initialization of
warnLogRateLimiter
has been moved to the constructor, which is a good practice for ensuring that the rate limiter is properly configured during object creation.
76-79
: New fields for monitoring.The new fields for monitoring (
isClientMonitorEnabled
,isClientMonitorJmxEnabled
,monitorExternalType
,monitorExternalExportPeriod
) enhance the monitoring capabilities of the class.
518-520
: New methodgetMonitorExternalType
.The new method
getMonitorExternalType
provides access to themonitorExternalType
field, enhancing the monitoring capabilities of the class.
539-541
: New methodgetMonitorExternalExportPeriod
.The new method
getMonitorExternalExportPeriod
provides access to themonitorExternalExportPeriod
field, enhancing the monitoring capabilities of the class.
553-555
: New methodisClientMonitorEnabled
.The new method
isClientMonitorEnabled
provides access to theisClientMonitorEnabled
field, enhancing the monitoring capabilities of the class.
565-567
: New methodisClientMonitorJmxEnabled
.The new method
isClientMonitorJmxEnabled
provides access to theisClientMonitorJmxEnabled
field, enhancing the monitoring capabilities of the class.
510-516
: New methodinitMonitorExternalType
.The new method
initMonitorExternalType
initializes themonitorExternalType
field, enhancing the monitoring capabilities of the class.
522-537
: New methodinitMonitorExternalCollectPeriod
.The new method
initMonitorExternalCollectPeriod
initializes themonitorExternalExportPeriod
field, enhancing the monitoring capabilities of the class.
544-551
: New methodinitClientMonitorEnabled
.The new method
initClientMonitorEnabled
initializes theisClientMonitorEnabled
field, enhancing the monitoring capabilities of the class.
557-564
: New methodinitClientMonitorJmxEnabled
.The new method
initClientMonitorJmxEnabled
initializes theisClientMonitorJmxEnabled
field, enhancing the monitoring capabilities of the class.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java (1)
56-56
: Changing access modifier toprotected
increases visibility.Changing
m_executorService
fromprivate
toprotected
allows subclasses to access it directly. Ensure this change is necessary and consider the potential risks of exposing this variable to subclasses.Verify that all subclasses handle
m_executorService
appropriately and do not introduce any concurrency issues.Verification successful
Changing access modifier to
protected
increases visibility.Changing
m_executorService
fromprivate
toprotected
allows subclasses to access it directly. Based on the search results, the current usages ofm_executorService
appear to be handled properly without introducing concurrency issues. Ensure that any subclasses handlem_executorService
appropriately to avoid potential concurrency issues.
- Files to review:
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloMetricsExporterTest.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `m_executorService` in subclasses. # Test: Search for the usage of `m_executorService` in subclasses. Expect: Proper handling without concurrency issues. rg --type java --pattern 'm_executorService' -A 5Length of output: 461
Script:
#!/bin/bash # Description: Verify usage of `m_executorService` in subclasses. # Test: Search for the usage of `m_executorService` in subclasses. Expect: Proper handling without concurrency issues. rg 'm_executorService' --type java -A 5Length of output: 16045
...ient/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java
Outdated
Show resolved
Hide resolved
...trip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java
Outdated
Show resolved
Hide resolved
...ip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java
Outdated
Show resolved
Hide resolved
...ip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java
Outdated
Show resolved
Hide resolved
.../main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java
Outdated
Show resolved
Hide resolved
Congratulation to submit the pr, |
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloNamespaceMonitorApi.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloRunningParamsMonitorApi.java
Outdated
Show resolved
Hide resolved
...-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloThreadPoolMonitorApi.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
Outdated
Show resolved
Hide resolved
...ip/framework/apollo/monitor/internal/collector/internal/DefaultApolloExceptionCollector.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (57)
- apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloRunningParamsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullRunningParamsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractMetricsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloRunningParamsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/internals/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEventPusher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMessageProducerManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/MeterType.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializer.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (8 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ExceptionUtil.java (1 hunks)
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1 hunks)
- apollo-client/src/main/resources/META-INF/services/com.ctrip.framework.apollo.tracer.spi.MessageProducerManager (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractApolloMetricsCollectorTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
Files not processed due to max files limit (7)
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/internals/DefaultMessageProducerManager.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/internals/cat/CatTransaction.java
- apollo-core/src/main/java/com/ctrip/framework/apollo/tracer/spi/MessageProducer.java
- apollo-plugin/apollo-plugin-client-prometheus/pom.xml
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/plugin/prometheus/PrometheusMetricExporter.java
- apollo-plugin/apollo-plugin-client-prometheus/src/main/resources/META-INF/services/com.ctrip.framework.apollo.monitor.internal.exporter.MetricsExporter
- apollo-plugin/pom.xml
Files skipped from review due to trivial changes (13)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullRunningParamsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ExceptionUtil.java
Files skipped from review as they are similar to previous changes (42)
- apollo-client/src/main/java/com/ctrip/framework/apollo/ConfigService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloRunningParamsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractMetricsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/MetricsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloExceptionCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloNamespaceCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloRunningParamsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultApolloThreadPoolCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/internal/DefaultMetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/MetricsExporterFactory.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/internals/DefaultMetricsExporterFactory.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEventPusher.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMessageProducerManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/JMXUtil.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/util/MeterType.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/spring/boot/ApolloApplicationContextInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json
- apollo-client/src/main/resources/META-INF/services/com.ctrip.framework.apollo.tracer.spi.MessageProducerManager
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/collector/AbstractApolloMetricsCollectorTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloMetricsExporterTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Additional comments not posted (5)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1)
27-27
: LGTM!The addition of the constant
APOLLO_AUTO_UPDATE_INJECTED_SPRING_PROPERTIES
is consistent with the existing naming conventions and enhances the configuration capabilities.apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (4)
168-168
: LGTM!The addition of the constant
APOLLO_CLIENT_MONITOR_ENABLED
is consistent with the existing naming conventions and enhances the monitoring capabilities.
173-173
: LGTM!The addition of the constant
APOLLO_CLIENT_MONITOR_JMX_ENABLED
is consistent with the existing naming conventions and enhances the monitoring capabilities.
178-178
: LGTM!The addition of the constant
APOLLO_CLIENT_MONITOR_EXTERNAL_TYPE
is consistent with the existing naming conventions and enhances the monitoring capabilities.
183-183
: LGTM!The addition of the constant
APOLLO_CLIENT_MONITOR_EXTERNAL_EXPORT_PERIOD
is consistent with the existing naming conventions and enhances the monitoring capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 12
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (32)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
Files skipped from review due to trivial changes (7)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java
Files skipped from review as they are similar to previous changes (14)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Additional comments not posted (77)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (2)
33-36
: LGTM!The
getCollectors
method is correctly implemented.
38-40
: LGTM!The
setCollectors
method is correctly implemented.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1)
27-70
: LGTM!The methods in the
ApolloClientBootstrapArgsMonitorApi
interface are correctly defined.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1)
30-57
: LGTM!The methods in the
ApolloClientNamespaceMonitorApi
interface are correctly defined.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (30)
28-29
: LGTM!The method signature is correct and adheres to best practices.
30-31
: LGTM!The method signature is correct and adheres to best practices.
32-33
: LGTM!The method signature is correct and adheres to best practices.
34-35
: LGTM!The method signature is correct and adheres to best practices.
36-37
: LGTM!The method signature is correct and adheres to best practices.
38-39
: LGTM!The method signature is correct and adheres to best practices.
40-41
: LGTM!The method signature is correct and adheres to best practices.
42-43
: LGTM!The method signature is correct and adheres to best practices.
44-45
: LGTM!The method signature is correct and adheres to best practices.
46-47
: LGTM!The method signature is correct and adheres to best practices.
48-49
: LGTM!The method signature is correct and adheres to best practices.
50-51
: LGTM!The method signature is correct and adheres to best practices.
52-53
: LGTM!The method signature is correct and adheres to best practices.
54-55
: LGTM!The method signature is correct and adheres to best practices.
56-57
: LGTM!The method signature is correct and adheres to best practices.
58-59
: LGTM!The method signature is correct and adheres to best practices.
60-61
: LGTM!The method signature is correct and adheres to best practices.
62-63
: LGTM!The method signature is correct and adheres to best practices.
64-65
: LGTM!The method signature is correct and adheres to best practices.
66-67
: LGTM!The method signature is correct and adheres to best practices.
68-69
: LGTM!The method signature is correct and adheres to best practices.
70-71
: LGTM!The method signature is correct and adheres to best practices.
72-73
: LGTM!The method signature is correct and adheres to best practices.
74-75
: LGTM!The method signature is correct and adheres to best practices.
76-77
: LGTM!The method signature is correct and adheres to best practices.
78-79
: LGTM!The method signature is correct and adheres to best practices.
80-81
: LGTM!The method signature is correct and adheres to best practices.
82-83
: LGTM!The method signature is correct and adheres to best practices.
84-85
: LGTM!The method signature is correct and adheres to best practices.
86-87
: LGTM!The method signature is correct and adheres to best practices.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (2)
88-90
: LGTM!The method is straightforward and correct.
110-112
: LGTM!The method is straightforward and correct.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (7)
50-58
: Constructor looks good.The constructor correctly initializes the thread pool executor services and calls the superclass constructor with specific metrics.
60-63
: Confirm the intentional no-op behavior.The
collect0
method is overridden to do nothing and return immediately. Ensure this behavior is intentional and documented.
66-73
: Method looks good.The
export0
method correctly exports thread pool metrics for all three executor services.
76-79
: Confirm the always-updated behavior.The
isSamplesUpdated
method returnstrue
, indicating that samples are always updated. Ensure this behavior is intentional and documented.
83-103
: Method looks good.The
exportThreadPoolMetrics
method correctly exports metrics for a given thread pool executor and name, and handles the gauge samples appropriately.
106-155
: Methods look good.The methods correctly return various metrics for the
remoteConfigRepositoryExecutorService
.
158-257
: Methods look good.The methods correctly return various metrics for the
abstractConfigExecutorService
andabstractConfigFileExecutorService
.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (7)
72-80
: Constructor looks good.The constructor correctly initializes the configuration maps and calls the superclass constructor with specific metrics.
84-114
: Method looks good.The
collect0
method correctly handles all event names and updates the corresponding namespace metrics.
117-131
: Method looks good.The
export0
method correctly exports namespace metrics and updates the gauge samples.
133-143
: Method looks good.The
updateCounterSample
method correctly updates the counter sample for a given key and namespace.
146-158
: Method looks good.The
updateGaugeSample
method correctly updates the gauge sample for a given key, namespace, and value, and applies the conversion function.
161-164
: Method looks good.The
getNamespaceMetrics
method correctly returns the namespace metrics map.
251-298
: Inner class looks good.The
NamespaceMetrics
inner class correctly provides methods to manage namespace metrics.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (22)
23-26
: Verify the intended behavior of returning an empty string.The method
getStartupParams
returns an empty string. Ensure this is the intended default behavior.
28-31
: Verify the intended behavior of returning an empty string.The method
getConfigServiceUrl
returns an empty string. Ensure this is the intended default behavior.
33-36
: Verify the intended behavior of returning an empty string.The method
getAccessKeySecret
returns an empty string. Ensure this is the intended default behavior.
38-41
: Verify the intended behavior of returning null.The method
getAutoUpdateInjectedSpringProperties
returns null. Ensure this is the intended default behavior.
43-46
: Verify the intended behavior of returning null.The method
getBootstrapEnabled
returns null. Ensure this is the intended default behavior.
48-51
: Verify the intended behavior of returning an empty string.The method
getBootstrapNamespaces
returns an empty string. Ensure this is the intended default behavior.
53-56
: Verify the intended behavior of returning null.The method
getBootstrapEagerLoadEnabled
returns null. Ensure this is the intended default behavior.
58-61
: Verify the intended behavior of returning null.The method
getOverrideSystemProperties
returns null. Ensure this is the intended default behavior.
63-66
: Verify the intended behavior of returning an empty string.The method
getCacheDir
returns an empty string. Ensure this is the intended default behavior.
68-71
: Verify the intended behavior of returning an empty string.The method
getCluster
returns an empty string. Ensure this is the intended default behavior.
73-76
: Verify the intended behavior of returning an empty string.The method
getConfigService
returns an empty string. Ensure this is the intended default behavior.
78-81
: Verify the intended behavior of returning null.The method
getClientMonitorEnabled
returns null. Ensure this is the intended default behavior.
83-86
: Verify the intended behavior of returning null.The method
getClientMonitorJmxEnabled
returns null. Ensure this is the intended default behavior.
88-91
: Verify the intended behavior of returning an empty string.The method
getClientMonitorExternalForm
returns an empty string. Ensure this is the intended default behavior.
93-96
: Verify the intended behavior of returning 0.The method
getClientMonitorExternalExportPeriod
returns 0. Ensure this is the intended default behavior.
98-101
: Verify the intended behavior of returning an empty string.The method
getApolloMeta
returns an empty string. Ensure this is the intended default behavior.
103-106
: Verify the intended behavior of returning an empty string.The method
getMetaLatestFreshTime
returns an empty string. Ensure this is the intended default behavior.
108-111
: Verify the intended behavior of returning null.The method
getPropertyNamesCacheEnable
returns null. Ensure this is the intended default behavior.
113-116
: Verify the intended behavior of returning null.The method
getPropertyOrderEnable
returns null. Ensure this is the intended default behavior.
118-121
: Verify the intended behavior of returning an empty string.The method
getVersion
returns an empty string. Ensure this is the intended default behavior.
123-126
: Verify the intended behavior of returning an empty string.The method
getEnv
returns an empty string. Ensure this is the intended default behavior.
128-131
: Verify the intended behavior of returning an empty string.The method
getAppId
returns an empty string. Ensure this is the intended default behavior.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (5)
47-49
: Verify the usage of constantsERROR_METRICS
.The constructor initializes the class with constants
ERROR_METRICS
. Ensure these constants are correctly defined and used.
51-54
: LGTM!The method
getExceptionNum
is correctly implemented.
56-59
: LGTM!The method
getExceptionList
is correctly implemented.
61-70
: Verify the exception handling logic.The method
collect0
collects exceptions from the event and updates the exception list and count. Ensure the logic correctly handles edge cases, such as null values and list overflow.
73-80
: Verify the export logic.The method
export0
exports the exception count to
...m/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java
Outdated
Show resolved
Hide resolved
...com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
Outdated
Show resolved
Hide resolved
...com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
Outdated
Show resolved
Hide resolved
...com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
Outdated
Show resolved
Hide resolved
...in/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
Outdated
Show resolved
Hide resolved
...in/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
Outdated
Show resolved
Hide resolved
...mework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
Outdated
Show resolved
Hide resolved
...mework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
Outdated
Show resolved
Hide resolved
...mework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
Outdated
Show resolved
Hide resolved
...mework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1)
19-23
: Class-level Javadoc is clear but could be expanded.The class-level Javadoc provides basic information, but it could be expanded to include more details about the purpose and usage of this class.
/** * Metrics constant. * * This class contains constants used for monitoring purposes within the Apollo client. * * @author Rawven */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (32)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
Files skipped from review due to trivial changes (9)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
Files skipped from review as they are similar to previous changes (22)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Additional comments not posted (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (4)
1-16
: Ensure the license header is up-to-date.The license header appears to be correct, but verify that it is up-to-date and consistent with the project's licensing terms.
17-17
: Package declaration looks good.The package declaration is appropriate for the file's purpose.
24-24
: Class declaration looks good.The class declaration is appropriate.
26-39
: Constant declarations are clear and follow naming conventions.The constant declarations are clear and follow standard naming conventions. Ensure that these constants are used consistently throughout the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1)
1-16
: Ensure the copyright year is updated.The copyright year should be updated to 2024.
- * Copyright 2022 Apollo Authors + * Copyright 2024 Apollo Authorsapollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1)
1-16
: Ensure the copyright year is updated.The copyright year should be updated to 2024.
- * Copyright 2022 Apollo Authors + * Copyright 2024 Apollo Authorsapollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxThreadPoolMonitor.java (1)
1-1
: Add a license header.The file is missing a license header. Ensure to add the appropriate license header at the top of the file.
+/* + * Copyright 2024 Apollo Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (36)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxBootstrapArgsMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxExceptionMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxNamespaceMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxThreadPoolMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
Files skipped from review due to trivial changes (5)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java
Files skipped from review as they are similar to previous changes (24)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Additional comments not posted (21)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxExceptionMonitor.java (3)
1-1
: Correct package declaration.The package declaration is appropriate for the file's location.
3-4
: Imports are correct.The imports are necessary and correctly used.
6-12
: Interface and method definition are correct.The interface is correctly annotated with
@MXBean
, and the methodgetExceptionDetails
is properly defined to return a list of strings.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxNamespaceMonitor.java (3)
1-1
: Correct package declaration.The package declaration is appropriate for the file's location.
3-4
: Imports are correct.The imports are necessary and correctly used.
6-32
: Interface and method definitions are correct.The interface is correctly annotated with
@MXBean
, and the methods are properly defined to return appropriate types. The methods cover various namespace-related metrics, which aligns with the purpose of the interface.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxBootstrapArgsMonitor.java (3)
1-1
: Correct package declaration.The package declaration is appropriate for the file's location.
3-3
: Imports are correct.The imports are necessary and correctly used.
5-51
: Interface and method definitions are correct.The interface is correctly annotated with
@MXBean
, and the methods are properly defined to return appropriate types. The methods cover various bootstrap arguments, which aligns with the purpose of the interface.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1)
26-30
: Interface definition looks good.The interface
ApolloClientNamespaceMonitorApi
is well-defined and extendsApolloJmxNamespaceMonitor
. The methodgetNamespaceMetrics
is appropriately included.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1)
26-38
: Class definition looks good.The class
NullClientExceptionMonitorApi
is well-defined and provides default implementations for the methodsgetExceptionList
andgetExceptionDetails
.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/jmx/ApolloJmxThreadPoolMonitor.java (1)
9-72
: Interface definition looks good.The interface
ApolloJmxThreadPoolMonitor
is well-defined and includes methods for monitoring various aspects of thread pools.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (9)
39-39
: LGTM!The constant
EXCEPTION_NUM
is appropriately defined.
41-41
: LGTM!The constant
MAX_EXCEPTIONS_SIZE
is appropriately defined.
42-43
: LGTM!The
exceptions
queue is appropriately defined with a maximum size.
45-45
: LGTM!The
exceptionNum
counter is appropriately defined.
47-49
: LGTM!The constructor correctly initializes the parent class with
ERROR_METRICS
.
51-54
: LGTM!The method
getExceptionList
correctly returns a new list containing the exceptions.
56-65
: LGTM!The method
collect0
correctly handles adding exceptions to the queue and updating the counter.
69-75
: LGTM!The method
export0
correctly updates thecounterSamples
with the current exception count.
78-84
: LGTM!The method
getExceptionDetails
correctly returns a new list containing the exception messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (36)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java (1 hunks)
Files skipped from review due to trivial changes (8)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
Files skipped from review as they are similar to previous changes (24)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloMetricsExporterFactoryTest.java
Additional comments not posted (42)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxExceptionApi.java (4)
1-16
: Ensure consistent documentation and licensing information.The license header is correctly included, which is good practice for open-source projects.
17-17
: Ensure proper package naming conventions.The package name
com.ctrip.framework.apollo.monitor.api.jmx
is appropriate and follows standard naming conventions.
19-21
: Ensure necessary imports are included.The imports for
List
andMXBean
are necessary and correctly included.
22-28
: Interface and method definition look good.The interface
ApolloJmxExceptionApi
and the methodgetExceptionDetails
are correctly defined. Ensure that the implementation of this method handles exceptions properly.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxNamespaceApi.java (4)
1-16
: Ensure consistent documentation and licensing information.The license header is correctly included, which is good practice for open-source projects.
17-17
: Ensure proper package naming conventions.The package name
com.ctrip.framework.apollo.monitor.api.jmx
is appropriate and follows standard naming conventions.
19-21
: Ensure necessary imports are included.The imports for
List
andMXBean
are necessary and correctly included.
22-48
: Interface and method definitions look good.The interface
ApolloJmxNamespaceApi
and the methods are correctly defined. Ensure that each method's implementation handles the respective operations efficiently and correctly.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxBootstrapArgsApi.java (4)
1-16
: Ensure consistent documentation and licensing information.The license header is correctly included, which is good practice for open-source projects.
17-17
: Ensure proper package naming conventions.The package name
com.ctrip.framework.apollo.monitor.api.jmx
is appropriate and follows standard naming conventions.
19-20
: Ensure necessary imports are included.The import for
MXBean
is necessary and correctly included.
21-67
: Interface and method definitions look good.The interface
ApolloJmxBootstrapArgsApi
and the methods are correctly defined. Ensure that each method's implementation handles the respective operations efficiently and correctly.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxThreadPoolApi.java (30)
28-28
: LGTM!The method
getRemoteConfigRepositoryThreadPoolActiveCount
is well-named and straightforward.
30-30
: LGTM!The method
getRemoteConfigRepositoryThreadPoolQueueSize
is well-named and straightforward.
32-32
: LGTM!The method
getRemoteConfigRepositoryThreadPoolCorePoolSize
is well-named and straightforward.
34-34
: LGTM!The method
getRemoteConfigRepositoryThreadPoolMaximumPoolSize
is well-named and straightforward.
36-36
: LGTM!The method
getRemoteConfigRepositoryThreadPoolPoolSize
is well-named and straightforward.
38-38
: LGTM!The method
getRemoteConfigRepositoryThreadPoolTaskCount
is well-named and straightforward.
40-40
: LGTM!The method
getRemoteConfigRepositoryThreadPoolCompletedTaskCount
is well-named and straightforward.
42-42
: LGTM!The method
getRemoteConfigRepositoryThreadPoolLargestPoolSize
is well-named and straightforward.
44-44
: LGTM!The method
getRemoteConfigRepositoryThreadPoolRemainingCapacity
is well-named and straightforward.
46-46
: LGTM!The method
getRemoteConfigRepositoryThreadPoolCurrentLoad
is well-named and straightforward.
48-48
: LGTM!The method
getAbstractConfigThreadPoolActiveCount
is well-named and straightforward.
50-50
: LGTM!The method
getAbstractConfigThreadPoolQueueSize
is well-named and straightforward.
52-52
: LGTM!The method
getAbstractConfigThreadPoolCorePoolSize
is well-named and straightforward.
54-54
: LGTM!The method
getAbstractConfigThreadPoolMaximumPoolSize
is well-named and straightforward.
56-56
: LGTM!The method
getAbstractConfigThreadPoolPoolSize
is well-named and straightforward.
58-58
: LGTM!The method
getAbstractConfigThreadPoolTaskCount
is well-named and straightforward.
60-60
: LGTM!The method
getAbstractConfigThreadPoolCompletedTaskCount
is well-named and straightforward.
62-62
: LGTM!The method
getAbstractConfigThreadPoolLargestPoolSize
is well-named and straightforward.
64-64
: LGTM!The method
getAbstractConfigThreadPoolRemainingCapacity
is well-named and straightforward.
66-66
: LGTM!The method
getAbstractConfigThreadPoolCurrentLoad
is well-named and straightforward.
69-69
: LGTM!The method
getAbstractConfigFileThreadPoolActiveCount
is well-named and straightforward.
71-71
: LGTM!The method
getAbstractConfigFileThreadPoolQueueSize
is well-named and straightforward.
73-73
: LGTM!The method
getAbstractConfigFileThreadPoolCorePoolSize
is well-named and straightforward.
75-75
: LGTM!The method
getAbstractConfigFileThreadPoolMaximumPoolSize
is well-named and straightforward.
77-77
: LGTM!The method
getAbstractConfigFileThreadPoolPoolSize
is well-named and straightforward.
79-79
: LGTM!The method
getAbstractConfigFileThreadPoolTaskCount
is well-named and straightforward.
81-81
: LGTM!The method
getAbstractConfigFileThreadPoolCompletedTaskCount
is well-named and straightforward.
83-83
: LGTM!The method
getAbstractConfigFileThreadPoolLargestPoolSize
is well-named and straightforward.
85-85
: LGTM!The method
getAbstractConfigFileThreadPoolRemainingCapacity
is well-named and straightforward.
87-87
: LGTM!The method
getAbstractConfigFileThreadPoolCurrentLoad
is well-named and straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 14
Outside diff range, codebase verification and nitpick comments (3)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1)
1-16
: Ensure the copyright year is up-to-date.The current year should be used in the copyright notice.
- * Copyright 2022 Apollo Authors + * Copyright 2024 Apollo Authorsapollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/plugin/prometheus/PrometheusMetricExporter.java (1)
1-16
: Ensure the copyright year is up-to-date.The current year should be used in the copyright notice.
- * Copyright 2022 Apollo Authors + * Copyright 2024 Apollo Authorsapollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1)
1-16
: Ensure the copyright year is up-to-date.The current year should be used in the copyright notice.
- * Copyright 2022 Apollo Authors + * Copyright 2024 Apollo Authors
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (40)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java (2 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java (1 hunks)
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/plugin/prometheus/PrometheusMetricExporter.java (1 hunks)
Files skipped from review due to trivial changes (8)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxExceptionApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultMetricsCollectorManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsModel.java
Files skipped from review as they are similar to previous changes (25)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/AbstractConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigServiceLocator.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultConfigManager.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/LocalFileConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/SimpleConfig.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/YamlConfigFile.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxBootstrapArgsApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/jmx/ApolloJmxThreadPoolApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientBootstrapArgsCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientExceptionCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientNamespaceCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/collector/impl/DefaultApolloClientThreadPoolCollector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultMetricsExporterFactory.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/MetricsEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/MessageProducerComposite.java
Additional context used
GitHub Check: codecov/patch
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
[warning] 31-31: apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java#L31
Added line #L31 was not covered by tests
[warning] 34-37: apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java#L34-L37
Added lines #L34 - L37 were not covered by tests
[warning] 41-41: apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java#L41
Added line #L41 was not covered by testsapollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
[warning] 48-48: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L48
Added line #L48 was not covered by tests
[warning] 57-63: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L57-L63
Added lines #L57 - L63 were not covered by tests
[warning] 68-68: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L68
Added line #L68 was not covered by tests
[warning] 74-74: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L74
Added line #L74 was not covered by tests
[warning] 76-77: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 80-80: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L80
Added line #L80 was not covered by tests
[warning] 82-82: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L82
Added line #L82 was not covered by tests
[warning] 85-85: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L85
Added line #L85 was not covered by tests
[warning] 87-88: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 92-92: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L92
Added line #L92 was not covered by tests
[warning] 94-94: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L94
Added line #L94 was not covered by tests
[warning] 99-99: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L99
Added line #L99 was not covered by tests
[warning] 101-101: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L101
Added line #L101 was not covered by tests
[warning] 103-103: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L103
Added line #L103 was not covered by tests
[warning] 105-105: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L105
Added line #L105 was not covered by tests
[warning] 107-107: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L107
Added line #L107 was not covered by tests
[warning] 109-109: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L109
Added line #L109 was not covered by tests
[warning] 111-111: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L111
Added line #L111 was not covered by tests
[warning] 120-120: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L120
Added line #L120 was not covered by tests
[warning] 124-124: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java#L124
Added line #L124 was not covered by testsapollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
[warning] 220-220: apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java#L220
Added line #L220 was not covered by tests
Additional comments not posted (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/MonitorConstant.java (1)
1-38
: LGTM! The constants are well-defined and follow best practices.The file is well-structured and the constants are clearly named.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1)
1-72
: LGTM! The class is well-defined and follows best practices.The file is well-structured and the methods are clearly defined.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1)
1-73
: LGTM! The class is well-defined and follows best practices.The file is well-structured and the methods are clearly defined.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java (1)
19-20
: LGTM!The new import statements for constants and utility classes are appropriate and necessary for the enhanced monitoring and exception handling functionalities.
...o-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigLongPollService.java
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
Outdated
Show resolved
Hide resolved
...o-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
089f16f
to
1808999
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1)
67-69
: Consider using a method reference.The lambda expression can be replaced with a method reference for better readability.
Apply this diff to refactor the code:
-executorMap.forEach((key, value) -> exportThreadPoolMetrics(value, key)); +executorMap.forEach(this::exportThreadPoolMetrics);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (28)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java (1 hunks)
Files skipped from review due to trivial changes (4)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java
Files skipped from review as they are similar to previous changes (20)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java
Additional comments not posted (26)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3)
20-24
: LGTM!The new import statements are necessary for the monitoring and metrics related bindings.
114-115
: LGTM!The new bindings for
ConfigMonitor
andApolloClientMonitorContext
are correctly configured and will enable monitoring of configuration changes and provide a context for client-side monitoring.
116-116
: LGTM!The new binding for
ApolloClientMetricsExporterFactory
is correctly configured and will enable exporting of client-side metrics using the default factory implementation.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (12)
1-17
: License header and package declaration look good!The code changes are approved.
19-30
: Imports look good!The code changes are approved.
32-45
: Class declaration and constructor look good!The code changes are approved.
47-50
:mBeanName
method looks good!The code changes are approved.
52-55
:isSupport
method looks good!The code changes are approved.
57-61
:collect
method looks good!The code changes are approved.
63-66
:isMetricsSampleUpdated
method looks good!The code changes are approved.
68-74
:export
method looks good!The code changes are approved.
76-86
:collect0
andexport0
method declarations look good!The code changes are approved.
89-108
:createOrUpdateGaugeSample
andcreateOrUpdateCounterSample
methods look good!The code changes are approved.
110-128
:createOrUpdateSample
method looks good!The code changes are approved.
130-139
:getTags
method looks good!The code changes are approved.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (5)
50-64
: LGTM!The constructor correctly initializes the
Map
ofApolloThreadPoolInfo
objects using the providedExecutorService
parameters.
99-121
: LGTM!The getter methods correctly retrieve
ApolloThreadPoolInfo
objects from theexecutorMap
.
127-129
: LGTM!The constructor correctly initializes the
executor
field using the providedThreadPoolExecutor
parameter.
131-165
: Ensure null-safe handling of executor metrics.Consider using
Optional
to handle potential null values more gracefully in all getter methods.
85-87
: Replace theMETRICS_THREAD_POOL_PARAMS
array with constants.The usage of the
METRICS_THREAD_POOL_PARAMS
array can be error-prone and hard to maintain. Consider replacing the array with constants for better readability and maintainability.Run the following script to verify the usage of the
METRICS_THREAD_POOL_PARAMS
array:Verification successful
Replace the
METRICS_THREAD_POOL_PARAMS
array with constants.The
METRICS_THREAD_POOL_PARAMS
array is only used in theDefaultApolloClientThreadPoolApi.java
file, specifically in thecreateOrUpdateGaugeSample
method. Replacing this array with constants will improve readability and maintainability, as accessing array elements by index can be error-prone. Consider refactoring this array into constants.
- Location of Definition:
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
- Location of Usage:
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `METRICS_THREAD_POOL_PARAMS` array. # Test: Search for the usage of the `METRICS_THREAD_POOL_PARAMS` array. Expect: Only occurrences in the `exportThreadPoolMetrics` method. rg --type java -w $'METRICS_THREAD_POOL_PARAMS'Length of output: 625
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (6)
52-54
: LGTM!The code changes are approved.
61-70
: LGTM!The code changes are approved.
72-103
: The empty case forAPOLLO_CLIENT_CONFIGMETA
with a non-English comment was flagged in a previous review and is still valid. Please address the comment by either clarifying the intent of the case or removing it if it is unnecessary.
177-180
: The empty method implementation was flagged in a previous review and is still valid. Please determine if the method is needed and implement it or remove it if unnecessary.
182-191
: LGTM!The code changes are approved.
193-196
: The method returning a null transaction was flagged in a previous review and is still valid. Please evaluate if this is appropriate or if a validTransaction
object should be returned.
...a/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java
Outdated
Show resolved
Hide resolved
de804bc
to
c60404e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (2)
72-103
: Clarify or remove the empty case in event handling.The method
handleTaggedEvent(String type, String name)
includes a case forAPOLLO_CLIENT_CONFIGMETA
that is empty and contains a comment in a non-English language. This could lead to confusion for developers who do not understand the language, and it's unclear why this case is necessary if it performs no action.Please clarify the intent of this case or remove it if it is unnecessary to avoid confusion and maintain clean code.
Consider translating the comment to English or providing additional context in the code to explain why this case is empty.
180-181
: Remove or implement the placeholder method.The method
logEvent(String type, String name, String status, String nameValuePairs)
is marked with a comment indicating it is ignored. If this method is not required, consider removing it to clean up the codebase. If it is meant to be implemented in the future, add a TODO comment to clarify that it is a work in progress.Consider either removing the method or adding a TODO comment to clarify future intentions:
- // ignore + // TODO: Implement this method or remove if not needed
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (37)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (8 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java (1 hunks)
Files skipped from review due to trivial changes (9)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java
Files skipped from review as they are similar to previous changes (23)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java
Additional comments not posted (12)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java (3)
29-32
: MethodgetName
is appropriately named.The existing suggestion to change the method name is not applicable here as the current name clearly reflects its functionality.
44-47
: MethodisMetricsSampleUpdated
is clear and appropriate.The method name accurately describes its functionality.
49-52
: Methodexport
is well-documented and functional.The method's documentation and implementation are clear and align with its intended purpose.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (1)
17-58
: Comprehensive Review of New JMX MBean InterfaceThe new
ApolloClientJmxNamespaceMBean
interface is well-structured and aligns with the PR's objectives to enhance observability within the Apollo configuration client. Here are some detailed observations and suggestions:
License and Documentation:
- The file is correctly licensed under the Apache License, Version 2.0.
- The authorship is clearly stated, which is good for tracking contributions.
Interface Design:
- The use of
@MXBean
is appropriate for JMX MBean registration.- Methods are well-documented, which aids in understanding their functionality.
Method Review:
getNamespaceMetricsString()
: ConvertsNamespaceMetrics
to aMap<String, Map<String, String>>
for JMX compatibility. This is a clever workaround since JMX does not support complex types directly.getNamespacePropertySize(String namespace)
: Returns the number of properties in a namespace, which is useful for monitoring configuration sizes.getConfigFileNamespaces()
: Lists all namespaces that have associated config files. This could be useful for auditing and tracking configurations.getNotFoundNamespaces()
: Lists namespaces that were queried but not found. This is crucial for identifying misconfigurations or typos in namespace usage.getTimeoutNamespaces()
: Lists namespaces where requests have timed out, indicating potential network or performance issues.Suggestions for Improvement:
- Error Handling: Consider adding methods to handle or report errors specifically related to JMX operations or namespace queries. This could enhance the robustness of the monitoring setup.
- Performance Metrics: Adding more detailed performance metrics (e.g., average response times, error rates) could provide deeper insights into the system's behavior under different conditions.
Overall, the interface is well-designed and meets the needs of enhanced observability for the Apollo client. It is recommended to proceed with implementing these interfaces and ensure thorough testing, especially focusing on edge cases and error conditions.
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (4)
20-24
: Import statements are correctly added.The new import statements are necessary for the classes used in the new bindings and are correctly placed.
114-114
: Binding forConfigMonitor
is correctly implemented.The binding of
ConfigMonitor
to its implementationDefaultConfigMonitor
usingSingleton
scope is appropriate for ensuring a single instance throughout the application lifecycle.
115-115
: Binding forApolloClientMonitorContext
is correctly implemented.The binding of
ApolloClientMonitorContext
as aSingleton
is appropriate, although it would be beneficial to ensure that this class is properly utilized across the application for consistent monitoring context management.
116-116
: Binding forApolloClientMetricsExporterFactory
is correctly implemented.The binding of
ApolloClientMetricsExporterFactory
toDefaultApolloClientMetricsExporterFactory
usingSingleton
scope ensures that metric export functionalities are centralized and managed efficiently.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (4)
1-16
: License Header: Correct and well-formatted.The license header is correctly placed and formatted, ensuring compliance with the Apache License, Version 2.0.
17-17
: Package Declaration: Correctly defined.The package declaration is appropriate for the file's purpose within the Apollo client's internal monitoring framework.
19-20
: Import Statements: Appropriate and necessary.The import statements for
ZoneId
andDateTimeFormatter
are necessary for the date formatting used in this file.
22-24
: Author Annotation: Clearly stated.The author annotation is clear, providing traceability for contributions which is useful in open-source projects.
...t/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
Outdated
Show resolved
Hide resolved
...a/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java
Outdated
Show resolved
Hide resolved
...a/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (37)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (8 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java (1 hunks)
Files skipped from review due to trivial changes (10)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java
Files skipped from review as they are similar to previous changes (23)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java
Additional comments not posted (11)
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3)
114-114
: Approved binding forConfigMonitor
.The binding of
ConfigMonitor
toDefaultConfigMonitor
is correctly implemented and enhances the monitoring capabilities.
115-115
: Approved binding forApolloClientMonitorContext
.The binding for
ApolloClientMonitorContext
is correctly implemented, ensuring proper context management in monitoring.
116-116
: Approved binding forApolloClientMetricsExporterFactory
.The binding of
ApolloClientMetricsExporterFactory
toDefaultApolloClientMetricsExporterFactory
is correctly implemented and crucial for exporting metrics.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (5)
43-45
: Approved constructor forAbstractApolloClientMonitorEventListener
.The constructor correctly initializes the
tag
field, which is essential for event handling.
48-50
: Approved methodgetName
.The method correctly returns the
tag
field, which is essential for identifying the event listener.
53-55
: Approved methodisSupport
.The method correctly checks if the event tag matches the listener's tag, which is crucial for event filtering.
58-61
: Approved methodcollect
.The method correctly handles the event collection and updates the
isUpdated
flag, which is crucial for tracking updates.
69-73
: Approved methodexport
.The method correctly compiles and returns the collected samples, which is crucial for data export.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (3)
36-45
: Constants in the 'common' category are well-defined and appropriate for their usage.The constants such as
NAMESPACE
,TIMESTAMP
, and others are clearly named and serve specific purposes within the Apollo monitoring context.
50-61
: Tracer constants are well-defined and crucial for detailed monitoring.Constants such as
APOLLO_CLIENT_CONFIGCHANGES
,APOLLO_CONFIG_EXCEPTION
, and others are appropriately named and categorized for tracing specific events and errors in the Apollo client.
67-93
: Collector tags and metrics constants are well-organized and serve their intended purposes effectively.Constants such as
TAG_ERROR
,TAG_NAMESPACE
, and various metrics constants likeMETRICS_NAMESPACE_LATEST_UPDATE_TIME
,METRICS_NAMESPACE_ITEM_NUM
are crucial for categorizing and quantifying monitoring data. They are appropriately named and organized.
...a/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java
Outdated
Show resolved
Hide resolved
...a/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java
Show resolved
Hide resolved
...t/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
Outdated
Show resolved
Hide resolved
...ava/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
Outdated
Show resolved
Hide resolved
...om/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
Outdated
Show resolved
Hide resolved
...ava/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java
Outdated
Show resolved
Hide resolved
...rip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java
Outdated
Show resolved
Hide resolved
...om/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java
Outdated
Show resolved
Hide resolved
f45a452
to
91a019e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (43)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxBootstrapArgsMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxExceptionMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (8 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java (1 hunks)
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/PrometheusApolloClientMetricsExporter.java (1 hunks)
Files skipped from review due to trivial changes (12)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxBootstrapArgsMBean.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java
Files skipped from review as they are similar to previous changes (26)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/PrometheusApolloClientMetricsExporter.java
Additional comments not posted (8)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxExceptionMBean.java (1)
26-36
: Well-structured JMX MBean interface.The
ApolloClientJmxExceptionMBean
interface is well-defined with appropriate methods for monitoring exceptions. The use of @mxbean is correctly applied, and the methods are suitably documented and implemented.apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1)
35-120
: Comprehensive and well-structured unit tests for metrics exporter.The
AbstractApolloClientMetricsExporterTest
class provides a thorough set of unit tests for the metrics exporter functionality. The use of Mockito for mocking and JUnit for assertions is appropriately applied. Each test method is well-documented and targets specific functionalities, ensuring comprehensive coverage and maintainability.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1)
30-134
: Well-implemented interface for monitoring bootstrap arguments.The
ApolloClientBootstrapArgsMonitorApi
interface is effectively designed with default methods that simplify the retrieval of configuration parameters. The methods are consistently implemented and documented, providing a robust foundation for further extension. The use of default methods enhances modularity and maintainability.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3)
114-114
: Approved: Binding for ConfigMonitorThe binding of
ConfigMonitor
toDefaultConfigMonitor
usingSingleton
scope is correctly configured and appropriate for its purpose.
116-116
: Approved: Binding for ApolloClientMetricsExporterFactoryThe binding of
ApolloClientMetricsExporterFactory
toDefaultApolloClientMetricsExporterFactory
usingSingleton
scope is correctly configured and suitable for its intended purpose.
115-115
: Approved but verify implementation: Binding for ApolloClientMonitorContextThe binding for
ApolloClientMonitorContext
is set toSingleton
scope. Ensure that the implementation or usage ofApolloClientMonitorContext
is correctly handled elsewhere in the project.Verification successful
Verified: Binding for ApolloClientMonitorContext
The
ApolloClientMonitorContext
is correctly implemented and utilized across the codebase. Its singleton binding is consistent with its usage pattern, and its functionality is covered by tests.
- The class is defined and used in multiple components, such as
ApolloClientMonitorEventPublisher
andDefaultConfigMonitor
.- It is instantiated using
ApolloInjector.getInstance
, confirming the singleton pattern.- Tests are present to verify its behavior.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of ApolloClientMonitorContext. # Test: Search for the implementation and usage of ApolloClientMonitorContext. Expect: Proper implementation and usage. rg --type java -A 5 $'ApolloClientMonitorContext'Length of output: 12581
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (2)
84-92
: Approved: Event handling in collect0 methodThe method
collect0
correctly handles configuration updates based on events, ensuring that only existing parameters are updated. This approach prevents potential errors from unhandled parameters.
99-107
: Approved: Getter methods for bootstrap argumentsThe getter methods
getBootstrapArgs
andgetBootstrapArgsString
provide essential access to the configured parameters. The null-safe handling of values when converting to strings is correctly implemented.
Comments failed to post (1)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1)
47-82: Approved with suggestion: Constructor of DefaultApolloClientBootstrapArgsApi
The constructor effectively initializes configuration parameters using both
ConfigUtil
and system properties. Consider adding error handling or logging for cases where system properties might not be set, to avoid runtime issues.
60958c4
to
df7c229
Compare
952d1f9
to
bd4f884
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1)
1-51
: Add documentation and unit tests.The class could benefit from additional documentation, such as Javadoc comments explaining the purpose and usage of the class and its methods. Also, consider adding unit tests to ensure the correctness of the
publish
andreset
methods.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1)
126-135
: Consider simplifying thegetTags
method using Java 8 streams.The
getTags
method can be simplified using Java 8 streams as follows:private Map<String, String> getTags(String[] tagKeys, String[] tagValues) { if (tagKeys != null && tagValues != null && tagKeys.length == tagValues.length) { return IntStream.range(0, tagKeys.length) .boxed() .collect(Collectors.toMap(i -> tagKeys[i], i -> tagValues[i])); } return Collections.emptyMap(); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (51)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java (3 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java (9 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultApolloClientMetricsExporterFactory.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/NullApolloClientMetricsExporter.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxBootstrapArgsMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxExceptionMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientExceptionMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMessageProducerComposite.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (4 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializerTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContextTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitorTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1 hunks)
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/PrometheusApolloClientMetricsExporter.java (1 hunks)
Files skipped from review due to trivial changes (3)
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorConstant.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/enums/MetricTypeEnums.java
- apollo-plugin/apollo-plugin-client-prometheus/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/PrometheusApolloClientMetricsExporter.java
Files skipped from review as they are similar to previous changes (34)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/DefaultInjector.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/RemoteConfigRepository.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientBootstrapArgsMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/api/ApolloClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEvent.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/DefaultApolloClientMetricsExporterFactory.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/exporter/impl/NullApolloClientMetricsExporter.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/ApolloClientMonitorEventListener.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientExceptionMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApi.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/GaugeModel.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMessageProducerComposite.java
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializerTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitorTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisherTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/AbstractApolloClientMetricsExporterTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/exporter/DefaultApolloClientMetricsExporterFactoryTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientBootstrapArgsApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientNamespaceApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientThreadPoolApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientNamespaceMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientThreadPoolMonitorApiTest.java
- apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/stress/ApolloClientMonitorStressTest.java
Additional comments not posted (48)
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxBootstrapArgsMBean.java (1)
1-35
: LGTM!The new
ApolloClientJmxBootstrapArgsMBean
interface follows Java naming conventions, has proper documentation, and defines a clear contract for retrieving bootstrap arguments as a string map. The use of a separate interface due to JMX limitations is well-explained in the comment.The code adheres to best practices by using the
@MXBean
annotation and defining the interface in a separate file for maintainability.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxExceptionMBean.java (3)
33-33
: LGTM!The method signature is correct and follows the Java naming convention. The return type is appropriate for the intended purpose of the method.
35-35
: LGTM!The method signature is correct and follows the Java naming convention. The return type is appropriate for the intended purpose of the method.
1-36
: Overall, the code in this file looks great!The interface and its methods are well-defined, follow Java naming conventions, and use appropriate return types. The
@MXBean
annotation is correctly used, and the file includes a standard Apache 2.0 license header.Great job on this new interface! Let me know if you have any further questions or concerns.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/NullClientBootstrapArgsMonitorApi.java (2)
1-34
: LGTM!The
NullClientBootstrapArgsMonitorApi
class correctly implements the null object pattern for theApolloClientBootstrapArgsMonitorApi
andApolloClientJmxBootstrapArgsMBean
interfaces. Returning an empty map fromgetBootstrapArgsString()
aligns with best practices to avoid null pointer exceptions.
30-33
: Suggestion from previous review still applies, but no action required.The past review comment suggesting the use of default methods in the interface to avoid implementing them in this class is still valid. However, as discussed in the comment thread, it's a design choice and may not always be applicable. Since the current implementation is correct and the discussion is already captured in the comments, no further action is needed.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/CounterModel.java (1)
24-53
: LGTM!The code segment looks good. The constructor and the
increase
method have the necessary validation logic as suggested in the past review comments. The other methods mentioned in the past review comments are not present in the current code, so those comments are not applicable.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java (1)
29-31
: Make static variables final.The static variables
COLLECTOR_MANAGER
andm_configUtil
should be madefinal
to enhance immutability and thread safety, as they are not intended to be reassigned.Apply this diff to make the static variables final:
- private static ApolloClientMonitorContext COLLECTOR_MANAGER = ApolloInjector.getInstance( + private static final ApolloClientMonitorContext COLLECTOR_MANAGER = ApolloInjector.getInstance( - private static ConfigUtil m_configUtil = ApolloInjector.getInstance(ConfigUtil.class); + private static final ConfigUtil m_configUtil = ApolloInjector.getInstance(ConfigUtil.class);apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/model/SampleModel.java (1)
1-73
: LGTM!The
SampleModel
class follows good practices like encapsulation and immutability. The fields are private, and thegetTags
method returns an unmodifiable map. The code looks clean and well-structured.The past review comments about making fields private, avoiding hardcoded prefixes, and returning an unmodifiable map are no longer applicable as the code has already addressed these concerns.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/DefaultConfigMonitor.java (5)
37-39
: LGTM!The method implementation is correct.
42-44
: LGTM!The method implementation is correct.
47-49
: LGTM!The method implementation is correct.
52-54
: LGTM!The method implementation is correct.
57-59
: LGTM!The method implementation is correct.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/jmx/mbean/ApolloClientJmxNamespaceMBean.java (2)
1-97
: LGTM!The rest of the code looks good. The interface and inner class are well-structured, and the methods have clear purposes. The code follows Java naming conventions and is properly documented.
34-34
: Consider using a custom class instead of a Map for better type safety and encapsulation.The past review comment by Anilople is still valid. Instead of returning a
Map<String, NamespaceMetricsString>
from thegetNamespaceMetricsString
method, consider creating a custom class that encapsulates the namespace metrics data. This would provide better type safety and encapsulation.For example, you could create a
NamespaceMetrics
class like this:public class NamespaceMetrics { private String namespace; private NamespaceMetricsString metrics; // Constructor, getters, and setters }Then, update the
getNamespaceMetricsString
method to return aList<NamespaceMetrics>
:List<NamespaceMetrics> getNamespaceMetrics();This way, you can encapsulate the namespace and its associated metrics within a single object, providing better structure and type safety.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/impl/DefaultApolloClientExceptionApi.java (4)
48-55
: LGTM!The constructor has been updated to use Guava's
EvictingQueue
for theexceptionsQueue
as per the past review comment. The implementation looks good.
58-60
: LGTM!The method correctly returns a new
ArrayList
containing the exceptions from theexceptionsQueue
.
63-65
: LGTM!The method correctly returns the value of
exceptionCountFromStartup
. The past review comment has been addressed andexceptionCountFromStartup
is now declared asfinal
.
68-75
: LGTM!The method correctly collects the
ApolloConfigException
from theApolloClientMonitorEvent
, adds it to theexceptionsQueue
, increments theexceptionCountFromStartup
, and creates or updates a counter sample. The implementation looks good.apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/listener/AbstractApolloClientMonitorEventListener.java (1)
1-137
: The code looks good and addresses the past review comment.The code uses
computeIfAbsent
in thecreateOrUpdateSample
method, which solves the thread-safety issue mentioned in the past review comment. The code is well-structured and follows good practices.apollo-client/src/test/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContextTest.java (3)
60-67
: LGTM!The test method correctly verifies the initial state of
ApolloClientMonitorContext
by checking that the default APIs and exporter are set to their respective null implementations.
69-82
: LGTM!The test method correctly verifies the setter and getter methods of
ApolloClientMonitorContext
by setting the APIs and exporter using the setter methods and checking that the getter methods return the same instances.
84-98
: LGTM!The test method correctly verifies the
getCollectors
method ofApolloClientMonitorContext
by setting the APIs using the setter methods and checking that thegetCollectors
method returns a list containing all the set APIs.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java (6)
52-61
: LGTM!The method now uses double-check locking to prevent concurrent initialization as suggested in the past review comments.
63-68
: LGTM!The method is correctly initializing the metrics event listener, metrics exporter, and JMX monitoring.
71-78
: LGTM!The method is correctly initializing JMX monitoring for the metrics collectors.
80-92
: LGTM!The method now stores the variables in the
ApolloClientMonitorContext
class as suggested in the past review comments.
94-103
: LGTM!The method is correctly initializing the metrics exporter.
105-121
: LGTM!The method is correctly initializing the message producer composite.
apollo-client/src/main/java/com/ctrip/framework/apollo/monitor/internal/tracer/ApolloClientMonitorMessageProducer.java (9)
53-55
: LGTM!The method logic is correct, and the implementation is accurate.
63-71
: LGTM!The method logic is correct, and the implementation is accurate. The events are handled appropriately based on the
type
parameter.
108-113
: LGTM!The method logic is correct, and the implementation is accurate.
115-121
: LGTM!The method logic is correct, and the implementation is accurate.
123-128
: LGTM!The method logic is correct, and the implementation is accurate.
130-135
: LGTM!The method logic is correct, and the implementation is accurate.
137-142
: LGTM!The method logic is correct, and the implementation is accurate.
144-149
: LGTM!The method logic is correct, and the implementation is accurate.
151-156
: LGTM!The method logic is correct, and the implementation is accurate.
apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (9)
259-266
: LGTM!The test method is correctly implemented and follows the standard pattern of setting a system property, creating a
ConfigUtil
instance, and asserting the expected value.
269-276
: LGTM!The test method is correctly implemented and follows the standard pattern of setting a system property, creating a
ConfigUtil
instance, and asserting the expected value.
279-286
: LGTM!The test method is correctly implemented and follows the standard pattern of setting an invalid system property, creating a
ConfigUtil
instance, and asserting the default value.
289-295
: LGTM!The test method is correctly implemented and follows the standard pattern of setting a system property, creating a
ConfigUtil
instance, and asserting the expected value.
298-304
: LGTM!The test method is correctly implemented and follows the standard pattern of clearing a system property, creating a
ConfigUtil
instance, and asserting the default value.
307-313
: LGTM!The test method is correctly implemented and follows the standard pattern of setting a system property, creating a
ConfigUtil
instance, and asserting the expected value.
316-322
: LGTM!The test method is correctly implemented and follows the standard pattern of clearing a system property, creating a
ConfigUtil
instance, and asserting the default value.
325-332
: LGTM!The test method is correctly implemented and follows the standard pattern of setting a system property, creating a
ConfigUtil
instance, and asserting the expected value.
335-342
: LGTM!The test method is correctly implemented and follows the standard pattern of setting an invalid system property, creating a
ConfigUtil
instance, and asserting the default value.
...ava/com/ctrip/framework/apollo/monitor/internal/event/ApolloClientMonitorEventPublisher.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/internals/ConfigMonitorInitializer.java
Outdated
Show resolved
Hide resolved
...nt/src/main/java/com/ctrip/framework/apollo/monitor/internal/ApolloClientMonitorContext.java
Outdated
Show resolved
Hide resolved
apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java
Outdated
Show resolved
Hide resolved
@apolloconfig/committers hello, guys, after a long time review and code change, I think it time to merge this pr. Need help, please take a look, is there something to watch out or change, or other suggestion? |
d8838a2
to
3245b61
Compare
What's the purpose of this PR
discussions OSPP2024
test cases Test
Which issue(s) this PR fixes:
Issue
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean test
to make sure this pull request doesn't break anything.CHANGES
log.Summary by CodeRabbit
Summary by CodeRabbit
New Features
ApolloClientBootstrapArgsMonitorApi
,ApolloClientNamespaceMonitorApi
, andApolloClientThreadPoolMonitorApi
to manage configuration parameters and resource metrics.Improvements
Bug Fixes
Tests