jiajunwang commented on a change in pull request #1383:
URL: https://github.com/apache/helix/pull/1383#discussion_r495186197
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_partition_check_http_requests_total");
Review comment:
How do we determine the metric type (gauge or counter) later when we
want to expose the MBean attribute to the monitoring system?
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck
performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String
instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}",
clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal =
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal =
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
+
+ try (final Timer.Context timer =
metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION)
Review comment:
metrics.timer(CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION).time() is
enough? You are not using the timer object in the try code block anyway, right?
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -166,12 +187,18 @@ public HelixDataAccessorWrapper(ZKHelixDataAccessor
dataAccessor, CustomRestClie
private Map<String, Boolean> getHealthStatusFromRest(String instance,
List<String> partitions,
RESTConfig restConfig, Map<String, String> customPayLoads) {
- try {
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal =
metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal =
metrics.counter(CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
Please check my comments for the instance accessor, the same here.
Please lazily get and simplify the code whenever possible.
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -231,14 +252,22 @@ private StoppableCheck
performHelixOwnInstanceCheck(String clusterId, String ins
private StoppableCheck performCustomInstanceCheck(String clusterId, String
instanceName,
String baseUrl, Map<String, String> customPayLoads) {
LOG.info("Perform instance level client side health checks for {}/{}",
clusterId, instanceName);
- try {
- return new StoppableCheck(
- _customRestClient.getInstanceStoppableCheck(baseUrl, customPayLoads),
+ MetricRegistry metrics = SharedMetricRegistries.getOrCreate(_namespace);
+ Counter requestsTotal =
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL);
+ Counter errorRequestsTotal =
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL);
Review comment:
It is lazily initialized, let's don't call it without a real error.
I would prefer just coding like the following, so you don't need the extra
local fields def.
metrics.counter(CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL).inc();
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/HelixRestServer.java
##########
@@ -222,7 +222,6 @@ public void shutdown() {
}
}
_jmxReporterList.forEach(JmxReporter::stop);
- _jmxReporterList.clear();
Review comment:
Why not clear?
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/HelixDataAccessorWrapper.java
##########
@@ -54,15 +60,30 @@
public static final String IS_HEALTHY_KEY = "IS_HEALTHY";
public static final String EXPIRY_KEY = "EXPIRE";
+ // Metric names for custom partition check
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_partition_check_http_requests_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUESTS_ERROR_TOTAL
= MetricRegistry
+ .name(InstanceService.class,
"custom_partition_check_http_requests_error_total");
+ private static final String CUSTOM_PARTITION_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class,
"custom_partition_check_http_request_duration");
+
private final Map<PropertyKey, HelixProperty> _propertyCache = new
HashMap<>();
private final Map<PropertyKey, List<String>> _batchNameCache = new
HashMap<>();
+ private String _namespace;
Review comment:
_namespace is not a must logic for the class, so I suggest adding a set
method for it instead of adding a constructor.
If the namespace is not set, then just skip the metrics logic.
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
You can get namespace from dataAccessor? Please try to ensure there is
only one source of truth.
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/server/service/InstanceServiceImpl.java
##########
@@ -58,9 +63,18 @@
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
private static final ExecutorService POOL = Executors.newCachedThreadPool();
+ // Metric names for custom instance check
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_requests_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUESTS_ERROR_TOTAL =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_requests_error_total");
+ private static final String CUSTOM_INSTANCE_CHECK_HTTP_REQUEST_DURATION =
+ MetricRegistry.name(InstanceService.class,
"custom_instance_check_http_request_duration");
+
private final HelixDataAccessorWrapper _dataAccessor;
Review comment:
Or do we share the same accessor across multiple service implementation?
If that's the case, we need to reconsider the design.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
