yifan-c commented on code in PR #111: URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1559895862
########## src/main/java/org/apache/cassandra/sidecar/metrics/RestoreMetrics.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics; + +import java.util.Objects; + +import com.codahale.metrics.DefaultSettableGauge; +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** + * Tracks metrics related to restore functionality provided by Sidecar. + */ +@Singleton +public class RestoreMetrics +{ + public static final String DOMAIN = "sidecar.restore"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Timer> sliceReplicationTime; + public final NamedMetric<Timer> jobCompletionTime; + public final NamedMetric<Meter> successfulJobs; + public final NamedMetric<Meter> failedJobs; + public final NamedMetric<DefaultSettableGauge<Integer>> activeJobs; + public final NamedMetric<Meter> tokenRefreshes; + public final NamedMetric<Meter> tokenUnauthorized; + public final NamedMetric<Meter> tokenExpired; + + @Inject + public RestoreMetrics(MetricRegistry metricRegistry) + { + this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null"); + + sliceReplicationTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_replication_time") + .build(); + jobCompletionTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("job_completion_time") + .build(); + successfulJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("successful_jobs") + .build(); + failedJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("failed_jobs") + .build(); Review Comment: I think they should be gauge, publish 1 to mark it as succeeded/failed. We do not need the rate. ########## src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceRestoreMetrics.java: ########## @@ -0,0 +1,135 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics.instance; + +import java.util.Objects; + +import com.codahale.metrics.DefaultSettableGauge; +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; +import org.apache.cassandra.sidecar.metrics.NamedMetric; + +import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX; + +/** + * {@link InstanceRestoreMetrics} contains metrics to track restore task for a Cassandra instance maintained by Sidecar. + */ +public class InstanceRestoreMetrics +{ + public static final String DOMAIN = INSTANCE_PREFIX + ".restore"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Timer> sliceCompletionTime; + public final NamedMetric<Timer> sliceImportTime; + public final NamedMetric<Timer> sliceStageTime; + public final NamedMetric<Timer> sliceUnzipTime; + public final NamedMetric<Timer> sliceValidationTime; + public final NamedMetric<Meter> sliceDownloadTimeouts; + public final NamedMetric<Meter> sliceDownloadRetries; + public final NamedMetric<Meter> sliceChecksumMismatches; + public final NamedMetric<DefaultSettableGauge<Integer>> sliceImportQueueLength; + public final NamedMetric<DefaultSettableGauge<Integer>> pendingSliceCount; + public final NamedMetric<DefaultSettableGauge<Long>> dataSSTableComponentSize; + public final NamedMetric<Timer> sliceDownloadTime; + public final NamedMetric<DefaultSettableGauge<Long>> sliceCompressedSizeInBytes; + public final NamedMetric<DefaultSettableGauge<Long>> sliceUncompressedSizeInBytes; + public final NamedMetric<Timer> longRunningRestoreHandler; + + public InstanceRestoreMetrics(MetricRegistry metricRegistry) + { + this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null"); + + sliceCompletionTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_completion_time") + .build(); + sliceImportTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_import_time") + .build(); + sliceStageTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_stage_time") + .build(); + sliceUnzipTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_unzip_time") + .build(); + sliceValidationTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_validation_time") + .build(); + sliceDownloadTimeouts + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("slice_download_timeouts") + .build(); + sliceDownloadRetries + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("slice_completion_retries") + .build(); + sliceChecksumMismatches + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("slice_checksum_mismatches") + .build(); + sliceImportQueueLength + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0))) + .withDomain(DOMAIN) + .withName("slice_import_queue_length") + .build(); + pendingSliceCount + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0))) + .withDomain(DOMAIN) + .withName("pending_slice_count") + .build(); + dataSSTableComponentSize + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0L))) + .withDomain(DOMAIN) + .withName("restore_size") + .addTag("component", "db") + .build(); + sliceDownloadTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_download_time") + .build(); + sliceCompressedSizeInBytes + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0L))) + .withDomain(DOMAIN) + .withName("slice_compressed_size_bytes") + .build(); + sliceUncompressedSizeInBytes + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0L))) + .withDomain(DOMAIN) + .withName("slice_uncompressed_size_bytes") + .build(); + longRunningRestoreHandler + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("long_running_restore_handler") Review Comment: rename to `SlowRestoreTask` ########## src/main/java/org/apache/cassandra/sidecar/metrics/RestoreMetrics.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics; + +import java.util.Objects; + +import com.codahale.metrics.DefaultSettableGauge; +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** + * Tracks metrics related to restore functionality provided by Sidecar. + */ +@Singleton +public class RestoreMetrics +{ + public static final String DOMAIN = "sidecar.restore"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Timer> sliceReplicationTime; + public final NamedMetric<Timer> jobCompletionTime; + public final NamedMetric<Meter> successfulJobs; + public final NamedMetric<Meter> failedJobs; + public final NamedMetric<DefaultSettableGauge<Integer>> activeJobs; + public final NamedMetric<Meter> tokenRefreshes; + public final NamedMetric<Meter> tokenUnauthorized; + public final NamedMetric<Meter> tokenExpired; + + @Inject + public RestoreMetrics(MetricRegistry metricRegistry) + { + this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null"); + + sliceReplicationTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_replication_time") + .build(); + jobCompletionTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("job_completion_time") + .build(); + successfulJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("successful_jobs") + .build(); + failedJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("failed_jobs") + .build(); + activeJobs + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0))) + .withDomain(DOMAIN) + .withName("active_jobs") + .build(); + tokenRefreshes + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("token_refreshes") + .build(); + tokenUnauthorized + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("token_unauthorized") + .build(); + tokenExpired + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("token_expired") + .build(); Review Comment: They should be gauge. ########## src/main/java/org/apache/cassandra/sidecar/metrics/RestoreMetrics.java: ########## @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics; + +import java.util.Objects; + +import com.codahale.metrics.DefaultSettableGauge; +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** + * Tracks metrics related to restore functionality provided by Sidecar. + */ +@Singleton +public class RestoreMetrics +{ + public static final String DOMAIN = "sidecar.restore"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Timer> sliceReplicationTime; + public final NamedMetric<Timer> jobCompletionTime; + public final NamedMetric<Meter> successfulJobs; + public final NamedMetric<Meter> failedJobs; + public final NamedMetric<DefaultSettableGauge<Integer>> activeJobs; + public final NamedMetric<Meter> tokenRefreshes; + public final NamedMetric<Meter> tokenUnauthorized; + public final NamedMetric<Meter> tokenExpired; + + @Inject + public RestoreMetrics(MetricRegistry metricRegistry) + { + this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null"); + + sliceReplicationTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("slice_replication_time") + .build(); + jobCompletionTime + = NamedMetric.builder(metricRegistry::timer) + .withDomain(DOMAIN) + .withName("job_completion_time") + .build(); + successfulJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("successful_jobs") + .build(); + failedJobs + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("failed_jobs") + .build(); + activeJobs + = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0))) + .withDomain(DOMAIN) + .withName("active_jobs") + .build(); + tokenRefreshes + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("token_refreshes") + .build(); Review Comment: `TokenRefreshed` ########## src/main/java/org/apache/cassandra/sidecar/metrics/SchemaMetrics.java: ########## @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics; + +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** + * Tracks metrics for {@link org.apache.cassandra.sidecar.db.schema.SidecarSchema} and other schema related handling + * done by Sidecar + */ +@Singleton +public class SchemaMetrics +{ + public static final String DOMAIN = "sidecar.schema"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Meter> failedInitializations; + public final NamedMetric<Meter> failedModifications; + + @Inject + public SchemaMetrics(MetricRegistry metricRegistry) + { + this.metricRegistry = metricRegistry; + + failedInitializations + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("failed_initializations") + .build(); + failedModifications + = NamedMetric.builder(metricRegistry::meter) + .withDomain(DOMAIN) + .withName("failed_modifications") + .build(); Review Comment: use gauge ########## src/main/java/org/apache/cassandra/sidecar/metrics/ResourceMetrics.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.metrics; + +import java.util.Objects; + +import com.codahale.metrics.Meter; +import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** + * Tracks resource metrics, for resources maintained by Sidecar. + */ +@Singleton +public class ResourceMetrics +{ + public static final String DOMAIN = "sidecar.resource"; + protected final MetricRegistry metricRegistry; + public final NamedMetric<Meter> shortLivedTasks; + public final NamedMetric<Meter> longTasks; + public final NamedMetric<Timer> shortTaskTimeTaken; + public final NamedMetric<Timer> longTaskTimeTaken; Review Comment: I think only timer is needed here. Timer has a meter inside. Can you also rename the metrics to `serviceTaskTimer` and `internalTaskTimer`? Service pool is to serve tasks from service API. Internal pool is to serve the background/operational/internal/ tasks. ########## src/main/java/org/apache/cassandra/sidecar/concurrent/ExecutorPools.java: ########## @@ -186,11 +208,31 @@ public long setTimer(long delay, Handler<Long> handler) public long setTimer(long delay, Handler<Long> handler, boolean ordered) { return vertx.setTimer(delay, id -> workerExecutor.executeBlocking(() -> { + recordTask(); + long startTime = System.nanoTime(); handler.handle(id); + recordTimeTaken(System.nanoTime() - startTime); return id; }, ordered)); } + private void recordTimeTaken(long duration) + { + if (metrics == null) + { + return; + } + + if (workerPoolName.equals(SERVICE_POOL)) + { + metrics.shortTaskTimeTaken.metric.update(duration, TimeUnit.NANOSECONDS); + } + else if (workerPoolName.equals(INTERNAL_POOL)) + { + metrics.longTaskTimeTaken.metric.update(duration, TimeUnit.NANOSECONDS); + } Review Comment: Using pool name within the pool implementation to determine the instance type is smelly. Let's not do that. You can change `TaskExecutorPool` to be abstract with abstract method `recordTimeTaken` declared. The abstract class does not need to hold a reference to the metrics. The following code snippet shows the idea. ```java public abstract static class TaskExecutorPool implements WorkerExecutor { private final Vertx vertx; private final WorkerExecutor workerExecutor; private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config) { ... } // record time taken (timer) already records the count. there is no need to declare recordTask(). protected abstract void recordTimeTaken(long duration); } private static class ServiceTaskExecutorPool extends TaskExecutorPool { private final ResourceMetrics metrics; private ServiceTaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config, ResourceMetrics metrics) { super(vertx, config); this.metrics = metrics; } @Override protected void recordTimeTaken(long duration) { // actual publishing code } } // similarly, add the pool for internal private static class InternalTaskExecutorPool extends TaskExecutorPool ``` ########## src/test/java/org/apache/cassandra/sidecar/concurrent/ExecutorPoolsTest.java: ########## @@ -27,31 +27,40 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import io.vertx.core.Vertx; +import io.vertx.junit5.VertxExtension; +import io.vertx.junit5.VertxTestContext; import org.apache.cassandra.sidecar.config.yaml.ServiceConfigurationImpl; +import org.apache.cassandra.sidecar.metrics.ResourceMetrics; +import static org.apache.cassandra.sidecar.utils.TestMetricUtils.registry; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; /** * Test {@link ExecutorPools} */ +@ExtendWith(VertxExtension.class) Review Comment: `VertxExtension` is not required. You can move `assertThat(metrics.longTasks.metric.getCount()).isEqualTo(200);` after countdown latch has passed, so you can get rid of VertxTestContext from the test. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
