feat: ConfigService cache record stats #5247
Conversation
dosubot
bot
added
the
size:L
WalkthroughThe changes in this pull request introduce a new method Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (2)
226-228: LGTM! Consider adding a brief Javadoc comment.The new method
isConfigServiceCacheStatsEnabled()is well-implemented and consistent with the existing codebase. It follows the established patterns for configuration methods in this class.Consider adding a brief Javadoc comment to describe the purpose of this method and the configuration property it uses. For example:
/** * Checks if the config service cache statistics are enabled. * @return true if cache stats are enabled, false otherwise */ public boolean isConfigServiceCacheStatsEnabled() { return getBooleanProperty("config-service.cache.stats.enabled", false); }
226-228: Well-integrated feature for cache statistics configuration.The addition of
isConfigServiceCacheStatsEnabled()aligns well with the PR objectives of introducing cache statistics recording for ConfigService. Its implementation is consistent with existing patterns in the class and maintains backward compatibility by defaulting tofalse.This method lays the groundwork for the cache monitoring feature mentioned in the linked issue #5246. It provides a configuration toggle that can be used to enable or disable cache statistics as needed, which addresses the requirement for a switchable monitoring feature.
As you implement the actual cache statistics recording functionality, consider the following:
- Ensure that the performance impact of recording statistics is minimal when enabled.
- Provide clear documentation on how to enable this feature and what metrics it covers (size, hit rate, eviction count, etc.).
- Consider implementing a mechanism to expose these statistics, perhaps through a metrics endpoint or integration with a monitoring system.
docs/zh/deployment/distributed-deployment-guide.md (1)
1566-1566: Improve formatting for JSON exampleThe JSON example for
apollo.release-history.retention.size.overrideconfiguration could be better formatted for readability. Consider using a code block with JSON syntax highlighting.Here's a suggested improvement:
-json -{ - "kl+bj+namespace1+bj": 10, - "kl+bj+namespace2+bj": 20 -} +```json +{ + "kl+bj+namespace1+bj": 10, + "kl+bj+namespace2+bj": 20 +} +```docs/en/deployment/distributed-deployment-guide.md (2)
1524-1534: New section on caching metrics looks good, with a minor suggestion.The new section about the
config-service.cache.stats.enabledconfiguration is well-written and informative. It clearly explains the purpose of the configuration, its default value, and how to enable it. However, I have a small suggestion to improve clarity:Consider rephrasing the sentence "This configuration works when
config-service.cache.stats.enabledis true" to "This configuration takes effect whenconfig-service.cache.enabledis true" for better clarity. This change would emphasize that the caching metrics feature is dependent on the main caching feature being enabled.Additionally, the reference to the monitoring metrics section is helpful. Great job on providing this cross-reference!
🧰 Tools
🪛 Markdownlint
1527-1527: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
1524-1534: Minor formatting improvements needed for consistency.There are a few minor formatting issues in the new section that should be addressed for consistency with the rest of the document:
- Remove the extra blank line between lines 1527 and 1528.
- Adjust the blockquote formatting for the note about restarting the config service. It should be consistent with other notes in the document. Consider using the following format:
> Note: The `config-service.cache.stats.enabled` configuration adjustment requires a restart of the config service to take effect.
- Remove the extra blank lines at the end of the section (lines 1533-1534).
These small changes will improve the overall consistency and readability of the document.
🧰 Tools
🪛 Markdownlint
1527-1527: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
153-231: Add unit tests for the new cache-building methods.Including unit tests for
buildConfigCache()andbuildConfigIdCache()will ensure that cache initialization and metric registration behave as expected under different configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (5 hunks)
- docs/en/deployment/distributed-deployment-guide.md (2 hunks)
- docs/zh/deployment/distributed-deployment-guide.md (2 hunks)
🧰 Additional context used
🪛 Markdownlint
docs/en/deployment/distributed-deployment-guide.md
1527-1527: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
docs/zh/deployment/distributed-deployment-guide.md
1469-1469: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
🔇 Additional comments (3)
docs/zh/deployment/distributed-deployment-guide.md (2)
1467-1474: New section on cache statistics configuration looks good.The new section 3.2.3.2 about
config-service.cache.stats.enabledis well-structured and provides clear information about the cache metric statistics feature. It includes the applicable version, the need for a restart, and the default value. The explanation of its functionality and reference to the monitoring section is helpful for users.🧰 Tools
🪛 Markdownlint
1469-1469: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
1467-1474: Overall, the new content enhances the documentationThe additions to the documentation provide valuable information about cache statistics configuration and fine-grained control over release history retention. These new features will be useful for users managing Apollo in production environments.
Also applies to: 1566-1566
🧰 Tools
🪛 Markdownlint
1469-1469: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
docs/en/deployment/distributed-deployment-guide.md (1)
1524-1534: Overall, great addition to the deployment guide!The new section on
config-service.cache.stats.enabledis a valuable addition to the Apollo distributed deployment guide. It provides clear information on how to enable caching metric statistics, which is crucial for monitoring and optimizing Apollo's performance.The content is well-integrated with the existing documentation and offers useful details such as:
- The version requirement (2.4.0 and above)
- The default setting and how to enable it
- A reference to the monitoring metrics section for more information
With the minor formatting adjustments suggested earlier, this addition will greatly enhance the guide's completeness and usefulness for Apollo administrators.
🧰 Tools
🪛 Markdownlint
1527-1527: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
| private void buildConfigCache() { | ||
| CacheBuilder configCacheBuilder = CacheBuilder.newBuilder() | ||
| .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_MINUTES, TimeUnit.MINUTES); | ||
| if (bizConfig.isConfigServiceCacheStatsEnabled()) { | ||
| configCacheBuilder.recordStats(); | ||
| } | ||
|
|
||
| configCache = configCacheBuilder.build(new CacheLoader<String, ConfigCacheEntry>() { | ||
| @Override | ||
| public ConfigCacheEntry load(String key) throws Exception { | ||
| List<String> namespaceInfo = ReleaseMessageKeyGenerator.messageToList(key); | ||
| if (CollectionUtils.isEmpty(namespaceInfo)) { | ||
| Tracer.logError( | ||
| new IllegalArgumentException(String.format("Invalid cache load key %s", key))); | ||
| return nullConfigCacheEntry; | ||
| } | ||
|
|
||
| Transaction transaction = Tracer.newTransaction(TRACER_EVENT_CACHE_LOAD, key); | ||
| try { | ||
| ReleaseMessage latestReleaseMessage = releaseMessageService.findLatestReleaseMessageForMessages(Lists | ||
| .newArrayList(key)); | ||
| Release latestRelease = releaseService.findLatestActiveRelease(namespaceInfo.get(0), namespaceInfo.get(1), | ||
| namespaceInfo.get(2)); | ||
|
|
||
| transaction.setStatus(Transaction.SUCCESS); | ||
|
|
||
| long notificationId = latestReleaseMessage == null ? ConfigConsts.NOTIFICATION_ID_PLACEHOLDER : latestReleaseMessage | ||
| .getId(); | ||
|
|
||
| if (notificationId == ConfigConsts.NOTIFICATION_ID_PLACEHOLDER && latestRelease == null) { | ||
| return nullConfigCacheEntry; | ||
| } | ||
|
|
||
| return new ConfigCacheEntry(notificationId, latestRelease); | ||
| } catch (Throwable ex) { | ||
| transaction.setStatus(ex); | ||
| throw ex; | ||
| } finally { | ||
| transaction.complete(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if (bizConfig.isConfigServiceCacheStatsEnabled()) { | ||
| GuavaCacheMetrics.monitor(meterRegistry, configCache, "config_cache"); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication between buildConfigCache() and buildConfigIdCache().
Both methods share similar logic for cache construction, statistics recording, and metrics monitoring. Extracting the common code into a generic helper method can improve maintainability and reduce redundancy.
Also applies to: 202-231
...ain/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGES.md (1)
14-14: LGTM! Consider adding a brief explanation of the feature's benefit.The new changelog entry accurately reflects the PR's main objective and is consistent with the format of other entries. Well done!
To provide more context for users, consider expanding the entry slightly:
-* [Feature: Add ConfigService cache record stats function](https://github.com/apolloconfig/apollo/pull/5247) +* [Feature: Add ConfigService cache record stats function for improved monitoring](https://github.com/apolloconfig/apollo/pull/5247)This minor addition clarifies the purpose of the new feature for readers of the changelog.
# Conflicts: # CHANGES.md
...ain/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
Outdated
Show resolved
Hide resolved
...ain/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (9)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (1)
74-74: LGTM: ConfigServiceWithCache constructor updated correctly.The
meterRegistryis now correctly passed to theConfigServiceWithCacheconstructor when the cache is enabled. This change aligns with the PR objective of adding cache statistics functionality.Consider adding a comment explaining the purpose of passing
meterRegistrytoConfigServiceWithCachefor better code documentation. For example:// Pass meterRegistry to enable cache statistics recording return new ConfigServiceWithCache(releaseService, releaseMessageService, grayReleaseRulesHolder(), bizConfig, meterRegistry);apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (2)
153-200: LGTM: New buildConfigCache() method with monitoring.The new
buildConfigCache()method effectively encapsulates the cache building logic and correctly implements cache stats and monitoring based on the configuration. This improves the overall structure of the class.However, there's still some code duplication between this method and
buildConfigIdCache(). Consider extracting the common cache building logic into a separate method to further improve maintainability.Would you like assistance in creating a generic cache building method to reduce duplication?
202-231: LGTM: New buildConfigIdCache() method with monitoring.The new
buildConfigIdCache()method effectively encapsulates the cache building logic for config IDs and correctly implements cache stats and monitoring based on the configuration. This improves the overall structure of the class.As mentioned in the previous comment, there's still some code duplication between this method and
buildConfigCache(). Consider extracting the common cache building logic into a separate method to further improve maintainability.Would you like assistance in creating a generic cache building method to reduce duplication between
buildConfigCache()andbuildConfigIdCache()?apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java (1)
Line range hint
31-77: Summary: Changes align with PR objective, consider additional tests.The modifications to
ConfigServiceWithCacheTestappropriately reflect the addition of cache statistics functionality toConfigServiceWithCache. The changes include:
- Importing
MeterRegistry- Adding a
MeterRegistrymock- Updating the
ConfigServiceWithCacheconstructor callThese changes are consistent and well-implemented. However, to ensure comprehensive test coverage:
Consider adding specific test cases for the new cache statistics functionality, such as:
- Verifying that the
MeterRegistryis correctly used to record cache statistics- Testing different scenarios of cache hits, misses, and evictions
- Checking the behavior when cache statistics are enabled vs. disabled
This will help ensure the robustness of the new feature and maintain the overall quality of the codebase.
apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheAndCacheKeyIgnoreCaseTest.java (1)
Line range hint
1-424: Consider adding tests for MeterRegistry functionality.While the changes to integrate
MeterRegistryare correct, there are no new tests specifically covering theMeterRegistryfunctionality inConfigServiceWithCache. Consider adding tests to ensure that the cache statistics are being recorded correctly.Would you like assistance in generating test cases for the
MeterRegistryfunctionality?docs/zh/deployment/distributed-deployment-guide.md (2)
1467-1475: New section on cache statistics configuration looks good, but could use minor improvements.The new section about
config-service.cache.stats.enabledis informative and well-structured. However, there are a few minor improvements we could make:
- The blank line inside the blockquote on line 1469 should be removed for consistency with Markdown best practices.
- Consider adding a brief explanation of why someone might want to enable cache statistics (e.g., for performance monitoring or troubleshooting).
Here's a suggested revision for the section:
#### 3.2.3.2 config-service.cache.stats.enabled - 是否开启缓存metric统计功能 > 适用于2.4.0及以上版本 > `config-service.cache.stats.enabled` 配置调整必须重启 config service 才能生效 该配置作用于`config-service.cache.stats.enabled`为 true 时，用于控制开启缓存统计功能。 默认为 false，即不会开启缓存统计功能，当配置为 true 时，开启缓存metric统计功能。 开启此功能可以帮助监控缓存性能和进行故障排查。 指标查看参考[监控相关-5.2 Metrics](zh/design/apollo-design#5.2-Metrics)，如`http://${someIp:somePort}/prometheus`🧰 Tools
🪛 Markdownlint
1469-1469: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
1567-1567: Minor formatting issue in JSON exampleThe JSON example for
apollo.release-history.retention.size.overrideis not properly formatted. It should be in a code block for better readability.Apply this change to improve the formatting:
-json -{ +```json +{ "kl+bj+namespace1+bj": 10, "kl+bj+namespace2+bj": 20 -} +} +```docs/en/deployment/distributed-deployment-guide.md (2)
1524-1533: New section on caching metric statistics looks good, but could use minor improvements.The new section about
config-service.cache.stats.enabledis informative and well-structured. However, there are a few minor points that could be improved:
- The blank line between the version information and the restart note could be removed for consistency with other similar sections.
- The explanation could be more concise and clear.
Here's a suggested revision:
#### 3.2.3.2 config-service.cache.stats.enabled - Whether to enable caching metric statistics function > For versions 2.4.0 and above > `config-service.cache.stats.enabled` configuration adjustment requires a restart of the config service to take effect. This configuration controls the cache statistics function when `config-service.cache.enabled` is true. The default is false. When set to true, it enables the cache metric statistics function. To view metrics, refer to [Monitoring related-5.2 Metrics](en/design/apollo-design#5.2-Metrics), such as `http://${someIp:somePort}/prometheus`🧰 Tools
🪛 Markdownlint
1527-1527: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
1629-1629: Minor formatting issue in JSON exampleThe JSON example for
apollo.release-history.retention.size.overrideis not properly formatted. It should be in a code block for better readability. Here's the corrected version:```json { "kl+bj+namespace1+bj": 10, "kl+bj+namespace2+bj": 20 }</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Files that changed from the base of the PR and between a5578fbf02dffbd0ba52e83e08a63645b7e22c0d and f4179d1495211177ed40568560e4d5904da62546. </details> <details> <summary>📒 Files selected for processing (6)</summary> * apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (3 hunks) * apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (5 hunks) * apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheAndCacheKeyIgnoreCaseTest.java (3 hunks) * apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java (3 hunks) * docs/en/deployment/distributed-deployment-guide.md (2 hunks) * docs/zh/deployment/distributed-deployment-guide.md (2 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> <details> <summary>docs/en/deployment/distributed-deployment-guide.md</summary><blockquote> 1527-1527: null Blank line inside blockquote (MD028, no-blanks-blockquote) </blockquote></details> <details> <summary>docs/zh/deployment/distributed-deployment-guide.md</summary><blockquote> 1469-1469: null Blank line inside blockquote (MD028, no-blanks-blockquote) </blockquote></details> </blockquote></details> </details> <details> <summary>🔇 Additional comments (16)</summary><blockquote> <details> <summary>apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/ConfigServiceAutoConfiguration.java (4)</summary><blockquote> `35-35`: **LGTM: MeterRegistry import added correctly.** The import of `io.micrometer.core.instrument.MeterRegistry` is appropriately placed and aligns with the PR objective of adding cache statistics functionality. --- `51-51`: **LGTM: MeterRegistry field added correctly.** The `private final MeterRegistry meterRegistry;` field is properly declared and placed consistently with other fields in the class. --- `54-57`: **LGTM: Constructor updated correctly to include MeterRegistry.** The constructor has been properly modified to accept the `MeterRegistry` parameter and assign it to the corresponding field. The changes maintain the existing structure and style of the constructor. Also applies to: 62-62 --- Line range hint `35-74`: **Overall assessment: Changes look good and align with PR objectives.** The modifications to `ConfigServiceAutoConfiguration.java` successfully integrate the `MeterRegistry` for cache statistics recording. The changes are well-structured, maintain consistency with the existing code, and fulfill the PR's objective of enhancing monitoring capabilities for the ConfigService cache. Key points: 1. MeterRegistry is properly imported and integrated. 2. The constructor is correctly updated to include the new dependency. 3. The `configService` bean method now passes the MeterRegistry to `ConfigServiceWithCache` when appropriate. These changes lay the groundwork for improved cache monitoring in Apollo ConfigService. </blockquote></details> <details> <summary>apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (4)</summary><blockquote> `38-39`: **LGTM: New imports for Micrometer metrics.** The addition of imports for `MeterRegistry` and `GuavaCacheMetrics` is appropriate for the new cache monitoring functionality. --- `69-69`: **LGTM: Constructor injection for MeterRegistry implemented.** The changes to the constructor and the addition of the `MeterRegistry` field address the previous review comment about using constructor injection. This modification enhances testability and adheres to dependency injection best practices. Also applies to: 80-81, 86-86 --- `92-93`: **LGTM: Refactored initialize() method.** The refactoring of the `initialize()` method to use separate methods for building each cache improves code organization and maintainability. This change enhances readability and makes the initialization process clearer. --- Line range hint `1-253`: **Overall LGTM: Successfully implemented cache monitoring with room for minor improvements.** This PR successfully addresses the objectives by implementing cache monitoring for the ConfigService. The changes improve the code structure, adhere to best practices, and enhance the maintainability of the `ConfigServiceWithCache` class. Key improvements: 1. Added Micrometer metrics for cache monitoring. 2. Implemented constructor injection for `MeterRegistry`. 3. Refactored cache initialization logic into separate methods. 4. Added configurable cache stats and monitoring. Suggestions for further improvement: 1. Consider extracting common cache building logic to reduce duplication between `buildConfigCache()` and `buildConfigIdCache()`. These changes align well with the PR objectives and the linked issue #5246, providing the requested monitoring capabilities for cache metrics such as size, hit rate, and eviction counts. </blockquote></details> <details> <summary>apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheTest.java (3)</summary><blockquote> `31-31`: **LGTM: Import statement for MeterRegistry added.** The addition of the `MeterRegistry` import aligns with the PR objective of introducing cache statistics recording for the `ConfigServiceWithCache`. --- `63-64`: **LGTM: MeterRegistry mock added.** The addition of the `MeterRegistry` mock is consistent with the PR objective and indicates that `ConfigServiceWithCache` now uses `MeterRegistry` for recording cache statistics. --- `77-77`: **LGTM: ConfigServiceWithCache constructor updated with MeterRegistry.** The constructor update is consistent with the PR objective of adding cache statistics functionality. To ensure comprehensive test coverage, please verify if: 1. Existing tests adequately cover the new cache statistics functionality. 2. Additional tests are needed to specifically test the interaction with `MeterRegistry`. You can use the following script to check for any new methods related to cache statistics: </blockquote></details> <details> <summary>apollo-configservice/src/test/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCacheAndCacheKeyIgnoreCaseTest.java (3)</summary><blockquote> `37-37`: **LGTM: Import statement added correctly.** The import for `MeterRegistry` is correctly added, which is necessary for the new dependency introduced in the test class. --- `66-66`: **LGTM: MeterRegistry field added correctly.** The `MeterRegistry` field is properly added and annotated with `@Mock`, consistent with other dependencies in the test class. --- `81-81`: **LGTM: ConfigServiceWithCache constructor call updated correctly.** The `ConfigServiceWithCache` constructor call has been properly updated to include the `meterRegistry` parameter, maintaining consistency with the changes in the `ConfigServiceWithCache` class. </blockquote></details> <details> <summary>docs/zh/deployment/distributed-deployment-guide.md (1)</summary><blockquote> Line range hint `1-1567`: **Overall, the documentation updates are valuable and well-structured** The changes to this documentation file provide important information about new configuration options in Apollo, particularly regarding cache statistics and release history retention. These additions will be helpful for users deploying Apollo in a distributed environment. A few minor suggestions for improvement: 1. Consider adding a brief introduction at the beginning of the file to highlight the new sections or major changes, helping readers quickly identify what's new. 2. Review the entire document for consistent formatting, especially in code blocks and JSON examples. 3. If possible, add some brief explanations or use cases for the new configuration options to help users understand when and why they might want to use them. These updates significantly enhance the documentation. With the minor suggested improvements, it will be even more valuable for Apollo users. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 1469-1469: null Blank line inside blockquote (MD028, no-blanks-blockquote) </blockquote></details> </details> </blockquote></details> <details> <summary>docs/en/deployment/distributed-deployment-guide.md (1)</summary><blockquote> Line range hint `1524-1629`: **Overall, the new additions to the documentation are valuable and well-integrated.** The new sections on caching metric statistics and release history retention provide important information for users deploying Apollo. These additions are well-structured and consistent with the rest of the document. With the minor improvements suggested earlier, this update will greatly enhance the deployment guide's usefulness. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint</summary><blockquote> 1527-1527: null Blank line inside blockquote (MD028, no-blanks-blockquote) </blockquote></details> </details> </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@nobodyiam I have solved these problems, please review again |
...ain/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java
Outdated
Show resolved
Hide resolved
# Conflicts: # CHANGES.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (1)
152-199: LGTM with suggestion: NewbuildConfigCache()method.The new
buildConfigCache()method effectively encapsulates the config cache initialization logic. The use ofbizConfigfor conditional logic and the addition of metrics monitoring are appropriate.However, there is some code duplication between this method and
buildConfigIdCache(). Consider extracting the common cache building logic into a separate method to improve maintainability.Consider refactoring the common cache building logic into a separate method to reduce duplication between
buildConfigCache()andbuildConfigIdCache(). For example:private <K, V> LoadingCache<K, V> buildCache(String cacheName, CacheLoader<K, V> loader) { CacheBuilder<Object, Object> builder = CacheBuilder.newBuilder() .expireAfterAccess(DEFAULT_EXPIRED_AFTER_ACCESS_IN_MINUTES, TimeUnit.MINUTES); if (bizConfig.isConfigServiceCacheStatsEnabled()) { builder.recordStats(); } LoadingCache<K, V> cache = builder.build(loader); if (bizConfig.isConfigServiceCacheStatsEnabled()) { GuavaCacheMetrics.monitor(meterRegistry, cache, cacheName); } return cache; }Then use this method in both
buildConfigCache()andbuildConfigIdCache().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- CHANGES.md (1 hunks)
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java (1 hunks)
- apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGES.md
- apollo-biz/src/main/java/com/ctrip/framework/apollo/biz/config/BizConfig.java
🧰 Additional context used
🔇 Additional comments (5)
apollo-configservice/src/main/java/com/ctrip/framework/apollo/configservice/service/config/ConfigServiceWithCache.java (5)
38-39: LGTM: New imports for Micrometer metrics.The addition of imports for
MeterRegistryandGuavaCacheMetricsis appropriate for the new cache monitoring functionality.
Line range hint
68-85: LGTM: Constructor injection for MeterRegistry.The addition of the
MeterRegistryfield and its inclusion in the constructor parameters is a good practice for dependency injection. This change aligns well with the new cache monitoring functionality.
91-92: LGTM: Refactored cache initialization.The refactoring of the
initialize()method to usebuildConfigCache()andbuildConfigIdCache()improves code organization and readability. This separation of concerns makes the initialization process more maintainable.
201-231: LGTM: NewbuildConfigIdCache()method.The new
buildConfigIdCache()method effectively encapsulates the config ID cache initialization logic. The use ofbizConfigfor conditional logic and the addition of metrics monitoring are appropriate.As mentioned in the previous comment, consider refactoring the common cache building logic to reduce duplication between this method and
buildConfigCache().
Line range hint
1-255: Overall assessment: Well-implemented cache monitoring functionality.The changes to
ConfigServiceWithCache.javasuccessfully introduce cache monitoring functionality using Micrometer. The code has been refactored to improve organization and readability, with the cache initialization logic now encapsulated in separate methods.Key improvements:
- Addition of
MeterRegistryfor metrics collection.- Refactored
@PostConstructmethod for better organization.- New methods
buildConfigCache()andbuildConfigIdCache()for clearer cache initialization.- Conditional enabling of cache stats and metrics monitoring.
Consider the suggested refactoring to further reduce code duplication between the cache building methods. This will enhance maintainability and adhere more closely to the DRY principle.
What's the purpose of this PR
ConfigService cache record stats
When the service is just started, we visit
curl 127.0.0.1:8080/prometheus | grep -E 'config_cache|config_id_cache'After some pull configuration requests, we visit
curl 127.0.0.1:8080/prometheus | grep -E 'config_cache|config_id_cache', and the cache monitoring indicators start recording dataWhich issue(s) this PR fixes:
Fixes #5246
Brief changelog
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates improve user experience and provide more control over configuration and deployment processes.