-
Notifications
You must be signed in to change notification settings - Fork 1.2k
KTOR-8276 fix micrometer metrics oom trigger #4737
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
KTOR-8276 fix micrometer metrics oom trigger #4737
Conversation
WalkthroughThe changes adjust how unregistered routes are reported in metrics. In the configuration class, the default value of Changes
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
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
🧹 Nitpick comments (2)
docker-compose.yml (2)
32-74: Consider extracting setup script to a separate fileThe bash script used in the setup service is quite long and complex. For better maintainability, consider extracting it to a separate shell script file and mounting it into the container.
This would make the docker-compose file cleaner and the setup script easier to maintain and modify.
27-27: Use specific version tags instead of environment variablesThe docker-compose file uses
${STACK_VERSION}for all Elastic Stack component versions. While this allows for consistent versioning, it requires setting this environment variable before running the compose file.Consider providing a default version in the docker-compose file itself or including a
.env.examplefile to document required environment variables.Also applies to: 84-84, 127-127, 161-161, 181-181, 202-202
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose.yml(1 hunks)ktor-server/ktor-server-plugins/ktor-server-metrics-micrometer/jvm/src/io/ktor/server/metrics/micrometer/MicrometerMetrics.kt(1 hunks)ktor-server/ktor-server-plugins/ktor-server-metrics-micrometer/jvm/test/io/ktor/server/metrics/micrometer/MicrometerMetricsTests.kt(1 hunks)
🔇 Additional comments (4)
ktor-server/ktor-server-plugins/ktor-server-metrics-micrometer/jvm/src/io/ktor/server/metrics/micrometer/MicrometerMetrics.kt (1)
69-69: Default for distinctNotRegisteredRoutes changed to falseThe default value of
distinctNotRegisteredRouteshas been changed fromtruetofalse, and the documentation has been updated to reflect this change. This means that unregistered routes will now be reported with a common "n/a" value in metrics instead of their actual request paths.This change helps prevent metric cardinality explosion when applications receive many different unregistered route requests, which could potentially overwhelm monitoring systems.
Also applies to: 75-75
ktor-server/ktor-server-plugins/ktor-server-metrics-micrometer/jvm/test/io/ktor/server/metrics/micrometer/MicrometerMetricsTests.kt (1)
247-247: Test assertion updated to match new default behaviorThe test assertion has been updated to expect "n/a" instead of the actual URI for unregistered routes, which aligns with the new default value of
distinctNotRegisteredRoutes = falsein the configuration.This change ensures that the test correctly verifies the new default behavior where unregistered routes are labeled as "n/a" in metrics.
docker-compose.yml (2)
1-216: New Elastic Stack deployment configurationA comprehensive
docker-compose.ymlfile has been added to set up an Elastic Stack environment, which can be useful for monitoring the metrics generated by the Ktor application. The setup includes:
- Elasticsearch for storing metrics data
- Kibana for visualization
- Metricbeat and Filebeat for collecting metrics and logs
- Logstash for processing logs
The configuration includes proper security settings with SSL certificates and authentication, along with health checks to ensure services start in the correct order.
While this file is not directly related to the bug fix in the Micrometer metrics plugin, it provides a valuable infrastructure component for monitoring and analyzing metrics in a real environment.
167-170: Verify volume mounts permissions in host environmentThe configuration includes several volume mounts that access sensitive system directories like
/var/run/docker.sock,/sys/fs/cgroup, and/proc. Ensure that these mounts are necessary and that appropriate permissions are set in the host environment.Mounting the Docker socket can be a security risk as it potentially gives containers root-equivalent access to the host. Consider restricting these permissions if possible.
Also applies to: 186-189
4323416 to
c6824e6
Compare
|
@bjhham |
Subsystem
Micrometor metrics
Motivation
KTOR-8276
Solution
Changed the default value of distinctNotRegisteredRoutes to false.
Since a test already exists for
no handler results in status 404, no route, and no exception if distinctNotRegisteredRoutes is falseno additional tests were written.