-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Collect metrics for application instances #4270
Conversation
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.
Hello @heowc, thanks for providing the PR. I have added some comments - please address. When it comes to actual metrics, I'll also request review from @jonatan-ivanov who specialises in it. Please also document the feature in docs/modules/ROOT/pages/spring-cloud-netflix.adoc
.
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ingframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsAutoConfiguration.java
Show resolved
Hide resolved
...ingframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsAutoConfiguration.java
Show resolved
Hide resolved
...t/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMetricsBinderTests.java
Outdated
Show resolved
Hide resolved
...t/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMetricsBinderTests.java
Outdated
Show resolved
Hide resolved
@jonatan-ivanov Could you please review the metrics-related code? |
...ingframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsAutoConfiguration.java
Show resolved
Hide resolved
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ava/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsBinder.java
Outdated
Show resolved
Hide resolved
...ver/src/test/java/org/springframework/cloud/netflix/eureka/server/InstanceRegistryTests.java
Outdated
Show resolved
Hide resolved
...t/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMetricsBinderTests.java
Outdated
Show resolved
Hide resolved
Thanks a lot @jonatan-ivanov. @heowc please address the comments and don't hesitate to let us know if you need additional help. |
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.
Thanks @heowc. Waiting for the rest of the changes we've discussed.
Thanks for updates, @heowc. It seems you're still working on it since there are TBD comments and not all the changes we've discussed have been implemented, I will not be reviewing it yet. Could you mark the PR as draft and then turn it back into regular PR once you're ready for review? Also, make sure to pull the changes from |
# Conflicts: # docs/modules/ROOT/partials/_configprops.adoc # spring-cloud-netflix-eureka-server/src/test/java/org/springframework/cloud/netflix/eureka/server/InstanceRegistryTests.java
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.
Thanks @heowc. This looks better. Have added some comments. Please address.
...main/java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMonitor.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Outdated
Show resolved
Hide resolved
...er/src/test/java/org/springframework/cloud/netflix/eureka/server/FixtureEurekaInstances.java
Outdated
Show resolved
Hide resolved
...ingframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMetricsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Show resolved
Hide resolved
@jonatan-ivanov Could you please review the reworked metrics? |
# Conflicts: # docs/modules/ROOT/partials/_configprops.adoc
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.
Thanks, @heowc. LGTM. Will still wait for an approval from @jonatan-ivanov before merging this, though.
...main/java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMonitor.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMonitor.java
Show resolved
Hide resolved
...main/java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMonitor.java
Show resolved
Hide resolved
...main/java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceMonitor.java
Outdated
Show resolved
Hide resolved
...java/org/springframework/cloud/netflix/eureka/server/metrics/EurekaInstanceTagsProvider.java
Outdated
Show resolved
Hide resolved
...g/springframework/cloud/netflix/eureka/server/metrics/DefaultEurekaInstanceTagsProvider.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/springframework/cloud/netflix/eureka/server/EurekaInstanceMonitorTests.java
Show resolved
Hide resolved
Thanks a lot @jonatan-ivanov. |
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.
Thanks @heowc. Have added a comment. Please address. Also, please remove the typos from doc entry that @jonatan-ivanov has indicated.
...gframework/cloud/netflix/eureka/server/EurekaInstanceMonitorWithCustomTagsProviderTests.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.
Thanks @heowc. LGTM.
Fixes #3685