chore: Enable netty metrics based on env var#36108
Conversation
WalkthroughThe changes introduce a new configuration property for metrics detail in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Application
participant CommonConfig
participant ReactorNettyConfiguration
User->>Application: Start Application
Application->>CommonConfig: Load Configuration
CommonConfig-->>Application: Return metricsDetail
Application->>ReactorNettyConfiguration: Initialize with CommonConfig
ReactorNettyConfiguration->>CommonConfig: Check metricsDetail
alt metricsDetail is true
ReactorNettyConfiguration->>Application: Enable Detailed Metrics
else metricsDetail is false
ReactorNettyConfiguration->>Application: Disable Detailed Metrics
end
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
nidhi-nair
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ReactorNettyConfiguration.java (1 hunks)
- app/server/appsmith-server/src/main/resources/application.properties (1 hunks)
Additional comments not posted (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/ReactorNettyConfiguration.java (3)
3-3: Great job using Lombok's@RequiredArgsConstructorannotation for dependency injection!The
@RequiredArgsConstructorannotation is a convenient way to generate a constructor with required arguments, which is a best practice for dependency injection. The import statement is correctly placed and used.
10-14: Excellent use of constructor injection and immutability!The
@RequiredArgsConstructorannotation generates a constructor that initializes thecommonConfigfield, enabling constructor injection. This is a good practice for dependency injection as it makes the dependencies explicit and ensures that the required dependencies are provided when creating an instance of the class.Furthermore, the
commonConfigfield is correctly declared asprivateandfinal, which promotes immutability and encapsulation. This prevents unintended modifications to the field and makes the code more maintainable.
18-20: Nicely done with the conditional metrics configuration!The changes in the
customizemethod introduce a conditional check based on theisMetricsDetailmethod ofCommonConfig. This allows for more granular control over metrics collection, as the metrics will only be enabled if the configuration explicitly allows for detailed metrics.The server customizers are added to the
NettyReactiveWebServerFactoryonly when the condition is met, which is a good practice for performance and resource management. The code changes are correctly implemented and follow the existing code style.app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)
70-71: Great job adding the newmetricsDetailconfiguration property! 👍The changes are well-structured and consistent with the existing
tracingDetailproperty. By allowing external configuration via the@Valueannotation, you've provided flexibility to control the metrics collection granularity.This enhancement in the application's monitoring capabilities will enable better observability and performance tuning. The default value of
falseensures that the metrics collection remains opt-in, which is a good practice.Keep up the excellent work in improving the application's configurability and observability! 🌟
app/server/appsmith-server/src/main/resources/application.properties (1)
126-126: Great job adding the new property for configuring metrics detail! 👍The new property
appsmith.micrometer.metrics.detail.enabledfollows the existing naming convention and provides a way to enable or disable detailed metrics collection. Using an environment variableAPPSMITH_ENABLE_METRICS_DETAILto set the value is a good practice as it allows overriding the default value offalse.This addition enhances the observability of the application by providing more control over the level of metrics detail collected. Keep up the good work!
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD 563db3a yet
Wed, 04 Sep 2024 06:50:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements