You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "sarankk (via GitHub)" <gi...@apache.org> on 2024/04/09 23:37:20 UTC
[PR] Record existing and additional metrics with dropwizard [cassandra-sidecar]
sarankk opened a new pull request, #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1568033908
##########
src/main/java/org/apache/cassandra/sidecar/routes/sstableuploads/SSTableImportHandler.java:
##########
@@ -162,18 +164,20 @@ protected SSTableImportRequest extractParamsOrThrow(RoutingContext context)
* @param importOptions the import options
* @return a future for the import
*/
- private Future<Void> importSSTablesAsync(SSTableImporter.ImportOptions importOptions)
+ private Future<Void> importSSTablesAsync(SSTableImporter.ImportOptions importOptions, SSTableImportMetrics metrics)
{
CassandraAdapterDelegate cassandra = metadataFetcher.delegate(importOptions.host());
if (cassandra == null)
{
+ metrics.cassandraUnavailable.metric.setValue(1);
return Future.failedFuture(cassandraServiceUnavailable());
}
TableOperations tableOperations = cassandra.tableOperations();
if (tableOperations == null)
{
+ metrics.cassandraUnavailable.metric.setValue(1);
Review Comment:
Removed `cassandraUnavailable`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561873587
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
We want to know for which SSTable components highDiskUsage metric is captured.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561874010
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -71,15 +79,21 @@ public UploadSSTableComponentMetrics(MetricRegistry metricRegistry, String sstab
NamedMetric.Tag componentTag = NamedMetric.Tag.of("component", sstableComponent);
rateLimitedCalls
- = NamedMetric.builder(metricRegistry::meter)
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
Review Comment:
Moved them out. But I feel for highDiskUsage it might be valuable to keep component detail
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1559857540
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceHealthMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks health related metrics of a Cassandra instance maintained by Sidecar.
+ */
+public class InstanceHealthMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".health";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<Meter> jmxDown;
+ public final NamedMetric<Meter> nativeDown;
Review Comment:
why are they meters? should operators care about the rate of it gets down? Should it be a gauge and publish the number of down instances?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1568036729
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/SSTableImportMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks metrics during SSTable import operation done in Cassandra instance
+ */
+public class SSTableImportMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".SSTableImport";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<DefaultSettableGauge<Integer>> cassandraUnavailable;
Review Comment:
Removed this metric
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561848104
##########
src/main/java/org/apache/cassandra/sidecar/metrics/Timer.java:
##########
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-package org.apache.cassandra.sidecar.stats;
+package org.apache.cassandra.sidecar.metrics;
Review Comment:
These methods were unused before too. I moved them as it is. @yifan-c
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#issuecomment-2060062903
Thanks @yifan-c for the feedback, addressed it.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1562869031
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
Discussed with Francisco, removing the metric per component.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561839297
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ }, false));
+ }
+
+ /**
+ * Like {@link Vertx#setTimer(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setTimer(delay, id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Delegate to {@link Vertx#cancelTimer(long)}
+ *
+ * @param id The id of the timer to cancel
+ * @return {@code true} if the timer was successfully cancelled, {@code false} otherwise
+ */
+ public boolean cancelTimer(long id)
+ {
+ return vertx.cancelTimer(id);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, ordered, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Callable<T> blockingCodeHandler, boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler), this::recordTimeTaken);
+ }
+
+ /**
+ * Records time taken for tasks executed by {@link TaskExecutorPool}
+ * @param duration time taken by a task
+ */
+ protected abstract void recordTimeTaken(long duration);
+
+ @Override
+ public void close(Handler<AsyncResult<Void>> handler)
+ {
+ throw new UnsupportedOperationException("Closing TaskExecutorPool is not supported!");
+ }
+
+ @Override
+ public Future<Void> close()
+ {
+ throw new UnsupportedOperationException("Closing TaskExecutorPool is not supported!");
+ }
+
+ Future<Void> closeInternal()
+ {
+ return workerExecutor == null
+ ? Future.succeededFuture()
+ : workerExecutor.close();
+ }
+
+ /**
+ * {@link ServiceTaskExecutorPool} is used for executing tasks that are short lived and not expected to block for
+ * too long, therefore will free up resources more quickly
+ */
+ static class ServiceTaskExecutorPool extends TaskExecutorPool
+ {
+ private final ResourceMetrics metrics;
+
+ ServiceTaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config, ResourceMetrics metrics)
+ {
+ super(vertx, config);
+ this.metrics = metrics;
+ }
+
+ @Override
+ protected void recordTimeTaken(long duration)
+ {
+ if (metrics == null)
Review Comment:
This one can be, because we might want to allow creation of `ExecutorPools` without metrics. Since `ExecutorPools` is also created from integration tests. Thats why we do the null check only here.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561911988
##########
src/main/java/org/apache/cassandra/sidecar/routes/sstableuploads/SSTableUploadHandler.java:
##########
@@ -127,7 +123,7 @@ public void handleInternal(RoutingContext context,
if (!limiter.tryAcquire())
{
String message = String.format("Concurrent upload limit (%d) exceeded", limiter.limit());
- componentMetrics.rateLimitedCalls.metric.mark();
+ componentMetrics.rateLimitedCalls.metric.setValue(1);
Review Comment:
Addressed this. Switched to using SSTable component name.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1562810632
##########
src/main/java/org/apache/cassandra/sidecar/metrics/Timer.java:
##########
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-package org.apache.cassandra.sidecar.stats;
+package org.apache.cassandra.sidecar.metrics;
Review Comment:
yep. let's keep them as is. Not related with this PR anyway
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#issuecomment-2050829729
Thanks @frankgh for the review. Addressed them. Modified to using SSTable component name instead of extension while tagging metrics
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561840020
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
It doesn't make much sense to track high disk usage on a per component basis. Maybe a drive makes sense, but it doesn't make sense for a specific component
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561837662
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
This is what we track specifically for upload handler
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561873587
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
We want to know for which SSTable components highDiskUsage metric is captured. We also track usable space in staging directory in general and that is a separate metric in `InstanceResourceMetrics`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1566615609
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -33,13 +34,26 @@
*/
public class UploadSSTableMetrics
{
- public static final String DOMAIN = INSTANCE_PREFIX + ".upload_sstable";
+ public static final String DOMAIN = INSTANCE_PREFIX + ".UploadSSTable";
protected final MetricRegistry metricRegistry;
protected final Map<String, UploadSSTableComponentMetrics> uploadComponentMetrics = new ConcurrentHashMap<>();
+ public final NamedMetric<Meter> totalBytesUploadedRate;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
Review Comment:
1. how about just rename it to `Throttled`? The same as the metric name.
2. The metric type needs to be `DeltaGauge`. We want to capture the number of throttled calls between the publishing intervals.
##########
src/main/java/org/apache/cassandra/sidecar/routes/sstableuploads/SSTableImportHandler.java:
##########
@@ -162,18 +164,20 @@ protected SSTableImportRequest extractParamsOrThrow(RoutingContext context)
* @param importOptions the import options
* @return a future for the import
*/
- private Future<Void> importSSTablesAsync(SSTableImporter.ImportOptions importOptions)
+ private Future<Void> importSSTablesAsync(SSTableImporter.ImportOptions importOptions, SSTableImportMetrics metrics)
{
CassandraAdapterDelegate cassandra = metadataFetcher.delegate(importOptions.host());
if (cassandra == null)
{
+ metrics.cassandraUnavailable.metric.setValue(1);
return Future.failedFuture(cassandraServiceUnavailable());
}
TableOperations tableOperations = cassandra.tableOperations();
if (tableOperations == null)
{
+ metrics.cassandraUnavailable.metric.setValue(1);
Review Comment:
I still think we should use `failedImports` only. But if keeping `cassandraUnavailable`, I want to call out that the metric value is never set to 0.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/SSTableImportMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks metrics during SSTable import operation done in Cassandra instance
+ */
+public class SSTableImportMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".SSTableImport";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<DefaultSettableGauge<Integer>> cassandraUnavailable;
Review Comment:
When `cassandraUnavailable`, the import fails, which can be classified under `failedImports`. Can we remove `cassandraUnavailable` from `SSTableImportMetrics`?
Besides that, `InstanceHealthMetrics` already captures `jmxDown`, which leads to `cassandraUnavailable` here.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/StreamSSTableMetrics.java:
##########
@@ -34,13 +34,26 @@
*/
public class StreamSSTableMetrics
{
- public static final String DOMAIN = INSTANCE_PREFIX + ".stream_sstable";
+ public static final String DOMAIN = INSTANCE_PREFIX + ".StreamSSTable";
protected final MetricRegistry metricRegistry;
protected final Map<String, StreamSSTableComponentMetrics> streamComponentMetrics = new ConcurrentHashMap<>();
+ public final NamedMetric<Meter> totalBytesStreamedRate;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
Review Comment:
same here. Rename and change type to `DeltaGauge`
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1566560085
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
Review Comment:
It is wrong w/o fulfilling the promise. I have a feedback commit (to be pushed) to address it.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561839663
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/VertxMetricsConfigurationImpl.java:
##########
@@ -32,34 +29,28 @@ public class VertxMetricsConfigurationImpl implements VertxMetricsConfiguration
public static final boolean DEFAULT_ENABLED = true;
public static final boolean DEFAULT_EXPOSE_VIA_JMX = false;
public static final String DEFAULT_JMX_DOMAIN_NAME = "sidecar.vertx.jmx_domain";
- public static final List<String> DEFAULT_MONITORED_SERVER_ROUTE_REGEXES = Collections.singletonList("/api/v1/.*");
Review Comment:
We decided to not use this. Instead use the include and exclude pattern we provide for filtering.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561840700
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/VertxMetricsConfigurationImpl.java:
##########
@@ -32,34 +29,28 @@ public class VertxMetricsConfigurationImpl implements VertxMetricsConfiguration
public static final boolean DEFAULT_ENABLED = true;
public static final boolean DEFAULT_EXPOSE_VIA_JMX = false;
public static final String DEFAULT_JMX_DOMAIN_NAME = "sidecar.vertx.jmx_domain";
- public static final List<String> DEFAULT_MONITORED_SERVER_ROUTE_REGEXES = Collections.singletonList("/api/v1/.*");
Review Comment:
should we add it in `sidecar.yaml` then?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561784788
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
Review Comment:
unrelated to the PR, but do we need to complete the `promise` here?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceRestoreMetrics.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.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<DefaultSettableGauge<Integer>> sliceDownloadTimeouts;
+ public final NamedMetric<DefaultSettableGauge<Integer>> sliceDownloadRetries;
+ public final NamedMetric<DefaultSettableGauge<Integer>> 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> slowRestoreTaskTime;
+
+ public InstanceRestoreMetrics(MetricRegistry metricRegistry)
+ {
+ this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null");
+
+ sliceCompletionTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceCompletionTime").build();
+ sliceImportTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceImportTime").build();
+ sliceStageTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceStageTime").build();
+ sliceUnzipTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceUnzipTime").build();
+ sliceValidationTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceValidationTime").build();
+ sliceDownloadTimeouts
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceDownloadTimeouts")
+ .build();
+ sliceDownloadRetries
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceCompletionRetries")
Review Comment:
The field is called `sliceDownloadRetries`, should the name match the field ?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceRestoreMetrics.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.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";
Review Comment:
NIT: for consistency with the uppercasing in other metrics
```suggestion
public static final String DOMAIN = INSTANCE_PREFIX + ".Restore";
```
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreProcessor.java:
##########
@@ -312,13 +325,17 @@ void decrementActiveSliceCount(RestoreSlice slice)
void captureImportQueueLength()
{
activeSliceCounterPerInstance.forEach((instanceId, counter) ->
- stats.captureSliceImportQueueLength(instanceId, counter.get()));
+ metadataFetcher.instance(instanceId)
Review Comment:
should capture be specific to the instance we are processing at the moment? i.e. just pass the `slice` as parameter and report it for the individual slice?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/SSTableImportMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks metrics during SSTable import operation done in Cassandra instance
+ */
+public class SSTableImportMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".SSTableImport";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<DefaultSettableGauge<Integer>> cassandraUnavailable;
Review Comment:
These metrics are a bit confusing. For `cassandraUnavailable` , it tracks on the handler but it doesn't track on the `SSTableImporter`. For `pendingImports`, it tracks on `SSTableImporter` . Should `cassandraUnavailable` track for the unavailable in `SSTableImporter` as well?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
nit
```suggestion
public final NamedMetric<DefaultSettableGauge<Integer>> highDiskUsage;
```
##########
src/main/java/org/apache/cassandra/sidecar/metrics/RestoreMetrics.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.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";
Review Comment:
NIT
```suggestion
public static final String DOMAIN = "Sidecar.Restore";
```
##########
src/main/java/org/apache/cassandra/sidecar/metrics/Timer.java:
##########
@@ -16,7 +16,7 @@
* limitations under the License.
*/
-package org.apache.cassandra.sidecar.stats;
+package org.apache.cassandra.sidecar.metrics;
Review Comment:
we have some unused methods in this class. Should we clean up?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceRestoreMetrics.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * 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.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<DefaultSettableGauge<Integer>> sliceDownloadTimeouts;
+ public final NamedMetric<DefaultSettableGauge<Integer>> sliceDownloadRetries;
+ public final NamedMetric<DefaultSettableGauge<Integer>> 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> slowRestoreTaskTime;
+
+ public InstanceRestoreMetrics(MetricRegistry metricRegistry)
+ {
+ this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric registry can not be null");
+
+ sliceCompletionTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceCompletionTime").build();
+ sliceImportTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceImportTime").build();
+ sliceStageTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceStageTime").build();
+ sliceUnzipTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceUnzipTime").build();
+ sliceValidationTime
+ = NamedMetric.builder(metricRegistry::timer).withDomain(DOMAIN).withName("SliceValidationTime").build();
+ sliceDownloadTimeouts
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceDownloadTimeouts")
+ .build();
+ sliceDownloadRetries
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceCompletionRetries")
+ .build();
+ sliceChecksumMismatches
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceChecksumMismatches")
+ .build();
+ sliceImportQueueLength
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("SliceImportQueueLength")
+ .build();
+ pendingSliceCount
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
+ .withDomain(DOMAIN)
+ .withName("PendingSliceCount")
+ .build();
+ dataSSTableComponentSize
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0L)))
+ .withDomain(DOMAIN)
+ .withName("RestoreDataSizeBytes")
+ .addTag("Component", "db")
Review Comment:
Should this be Data.db instead? db is not unique for the data file
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/VertxMetricsConfigurationImpl.java:
##########
@@ -32,34 +29,28 @@ public class VertxMetricsConfigurationImpl implements VertxMetricsConfiguration
public static final boolean DEFAULT_ENABLED = true;
public static final boolean DEFAULT_EXPOSE_VIA_JMX = false;
public static final String DEFAULT_JMX_DOMAIN_NAME = "sidecar.vertx.jmx_domain";
- public static final List<String> DEFAULT_MONITORED_SERVER_ROUTE_REGEXES = Collections.singletonList("/api/v1/.*");
Review Comment:
where will this go?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/SSTableImportMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks metrics during SSTable import operation done in Cassandra instance
+ */
+public class SSTableImportMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".SSTableImport";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<DefaultSettableGauge<Integer>> cassandraUnavailable;
+ public final NamedMetric<DefaultSettableGauge<Integer>> pendingImports;
Review Comment:
I think it makes sense to track successful imports as well, what do you think?
##########
src/main/java/org/apache/cassandra/sidecar/restore/RestoreProcessor.java:
##########
@@ -141,21 +146,26 @@ public void execute(Promise<Void> promise)
sliceQueue.captureImportQueueLength();
RestoreSliceHandler task = slice.toAsyncTask(s3ClientPool, pool, importer,
requiredUsableSpacePercentage,
- sliceDatabaseAccessor, stats,
- restoreJobUtil);
+ sliceDatabaseAccessor,
+ restoreJobUtil,
+ restoreMetrics);
activeTasks.add(task);
pool.executeBlocking(task, false) // unordered; run in parallel
.onSuccess(restoreSlice -> {
int instanceId = slice.owner().id();
Review Comment:
`instanceId` is now unused
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ }, false));
+ }
+
+ /**
+ * Like {@link Vertx#setTimer(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setTimer(delay, id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Delegate to {@link Vertx#cancelTimer(long)}
+ *
+ * @param id The id of the timer to cancel
+ * @return {@code true} if the timer was successfully cancelled, {@code false} otherwise
+ */
+ public boolean cancelTimer(long id)
+ {
+ return vertx.cancelTimer(id);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, ordered, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Callable<T> blockingCodeHandler, boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler), this::recordTimeTaken);
+ }
+
+ /**
+ * Records time taken for tasks executed by {@link TaskExecutorPool}
+ * @param duration time taken by a task
+ */
+ protected abstract void recordTimeTaken(long duration);
Review Comment:
```suggestion
protected abstract void recordTimeTaken(long durationNanos);
```
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -159,16 +159,45 @@ protected ImportQueue initializeQueue(String key)
*/
private void processPendingImports(Long timerId)
{
- for (ImportQueue queue : importQueuePerHost.values())
+ for (Map.Entry<String, ImportQueue> entry : importQueuePerHost.entrySet())
{
+ ImportQueue queue = entry.getValue();
if (!queue.isEmpty())
{
+ recordPendingImports(hostFromKey(entry.getKey()), entry.getValue().size());
executorPools.internal()
.executeBlocking(p -> maybeDrainImportQueue(queue));
}
}
}
+ private void recordPendingImports(String host, int pendingImports)
+ {
+ if (host != null)
+ {
+ metadataFetcher.instance(host)
+ .metrics()
+ .sstableImport().pendingImports.metric.setValue(pendingImports);
+ }
+ }
+
+ /**
+ * Host is extracted from key for publishing Cassandra instance specific metrics related to {@link ImportQueue}
+ *
+ * @param key a key for one of the import queues
+ * @return host for which the import queue is maintained
+ */
+ protected String hostFromKey(String key)
+ {
+ String[] keyParts = key.split("\\$");
Review Comment:
do we really want to parse the key here? is there a better way to do this?
##########
src/main/java/org/apache/cassandra/sidecar/routes/sstableuploads/SSTableUploadHandler.java:
##########
@@ -127,7 +123,7 @@ public void handleInternal(RoutingContext context,
if (!limiter.tryAcquire())
{
String message = String.format("Concurrent upload limit (%d) exceeded", limiter.limit());
- componentMetrics.rateLimitedCalls.metric.mark();
+ componentMetrics.rateLimitedCalls.metric.setValue(1);
Review Comment:
The component metrics seem wrong or misleading. When I think of components I think of SSTable Components. https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/io/sstable/format/big/BigFormat.java#L206 But you are really tracking by extension, which is different than component. I think we should either rename the metric, or if we really want to track SSTable components, we should properly determine the SSTable component.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -55,8 +62,9 @@ public static class UploadSSTableComponentMetrics
{
protected final MetricRegistry metricRegistry;
public final String sstableComponent;
- public final NamedMetric<Meter> rateLimitedCalls;
- public final NamedMetric<Meter> diskUsageHigh;
+ public final NamedMetric<DefaultSettableGauge<Integer>> rateLimitedCalls;
+ public final NamedMetric<DefaultSettableGauge<Integer>> diskUsageHigh;
Review Comment:
Why is the disk usage associated to the SSTable component? Wouldn't this be a metric that tracks the staging directory instead?
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ }, false));
+ }
+
+ /**
+ * Like {@link Vertx#setTimer(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setTimer(delay, id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Delegate to {@link Vertx#cancelTimer(long)}
+ *
+ * @param id The id of the timer to cancel
+ * @return {@code true} if the timer was successfully cancelled, {@code false} otherwise
+ */
+ public boolean cancelTimer(long id)
+ {
+ return vertx.cancelTimer(id);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, ordered, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Callable<T> blockingCodeHandler, boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler), this::recordTimeTaken);
+ }
+
+ /**
+ * Records time taken for tasks executed by {@link TaskExecutorPool}
+ * @param duration time taken by a task
Review Comment:
we are missing the units here
##########
src/main/java/org/apache/cassandra/sidecar/concurrent/TaskExecutorPool.java:
##########
@@ -0,0 +1,277 @@
+/*
+ * 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.concurrent;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.TimeUnit;
+
+import io.vertx.core.AsyncResult;
+import io.vertx.core.Future;
+import io.vertx.core.Handler;
+import io.vertx.core.Promise;
+import io.vertx.core.Vertx;
+import io.vertx.core.WorkerExecutor;
+import org.apache.cassandra.sidecar.config.WorkerPoolConfiguration;
+import org.apache.cassandra.sidecar.metrics.ResourceMetrics;
+import org.apache.cassandra.sidecar.metrics.Timer;
+
+/**
+ * A pool of executors backed by {@link WorkerExecutor} and {@link Vertx}
+ */
+public abstract class TaskExecutorPool implements WorkerExecutor
+{
+ private final Vertx vertx;
+ private final WorkerExecutor workerExecutor;
+
+ private TaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config)
+ {
+ this.vertx = vertx;
+ this.workerExecutor = vertx.createSharedWorkerExecutor(config.workerPoolName(),
+ config.workerPoolSize(),
+ config.workerMaxExecutionTimeMillis(),
+ TimeUnit.MILLISECONDS);
+ }
+
+ /**
+ * Like {@link #setPeriodic(long, Handler, boolean)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler)
+ {
+ return setPeriodic(delay, handler, false);
+ }
+
+ /**
+ * Like {@link Vertx#setPeriodic(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setPeriodic(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return setPeriodic(delay, delay, handler, ordered);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler)
+ {
+ return setPeriodic(initialDelay, delay, handler, false);
+ }
+
+ /**
+ * Set a periodic timer to fire every {@code delay} milliseconds with initial delay, at which point
+ * {@code handler} will be called with the id of the timer.
+ *
+ * @param initialDelay the initial delay in milliseconds
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true then executeBlocking is called several times on the same context, the
+ * executions for that context will be executed serially, not in parallel. if false
+ * then they will be no ordering guarantees
+ * @return the unique ID of the timer
+ */
+ public long setPeriodic(long initialDelay, long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setPeriodic(initialDelay,
+ delay,
+ id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Like {@link #setTimer(long, Handler)} with the handler guaranteed to be executed in a
+ * worker thread and ordered = false.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler)
+ {
+ return vertx.setTimer(delay, id ->
+ workerExecutor.executeBlocking(promise -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ }, false));
+ }
+
+ /**
+ * Like {@link Vertx#setTimer(long, Handler)} with the handler guaranteed to be executed in a worker thread.
+ *
+ * @param delay the delay in milliseconds, after which the timer will fire
+ * @param handler the handler that will be called with the timer ID when the timer fires
+ * @param ordered if true and if executeBlocking is called several times on the same context, the executions
+ * for that context will be executed serially, not in parallel. The periodic scheduled
+ * executions could be delayed if the prior execution on the same context is taking longer.
+ * If false then they will be no ordering guarantees
+ * @return the unique identifier for the timer
+ */
+ public long setTimer(long delay, Handler<Long> handler, boolean ordered)
+ {
+ return vertx.setTimer(delay, id -> workerExecutor.executeBlocking(() -> {
+ long startTime = System.nanoTime();
+ handler.handle(id);
+ recordTimeTaken(System.nanoTime() - startTime);
+ return id;
+ }, ordered));
+ }
+
+ /**
+ * Delegate to {@link Vertx#cancelTimer(long)}
+ *
+ * @param id The id of the timer to cancel
+ * @return {@code true} if the timer was successfully cancelled, {@code false} otherwise
+ */
+ public boolean cancelTimer(long id)
+ {
+ return vertx.cancelTimer(id);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, ordered, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Callable<T> blockingCodeHandler, boolean ordered)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler, ordered), this::recordTimeTaken);
+ }
+
+ @Override
+ public <T> void executeBlocking(Handler<Promise<T>> blockingCodeHandler,
+ Handler<AsyncResult<T>> asyncResultHandler)
+ {
+ workerExecutor.executeBlocking(blockingCodeHandler, asyncResultHandler);
+ }
+
+ @Override
+ public <T> Future<T> executeBlocking(Handler<Promise<T>> blockingCodeHandler)
+ {
+ return Timer.measureTimeTaken(workerExecutor.executeBlocking(blockingCodeHandler), this::recordTimeTaken);
+ }
+
+ /**
+ * Records time taken for tasks executed by {@link TaskExecutorPool}
+ * @param duration time taken by a task
+ */
+ protected abstract void recordTimeTaken(long duration);
+
+ @Override
+ public void close(Handler<AsyncResult<Void>> handler)
+ {
+ throw new UnsupportedOperationException("Closing TaskExecutorPool is not supported!");
+ }
+
+ @Override
+ public Future<Void> close()
+ {
+ throw new UnsupportedOperationException("Closing TaskExecutorPool is not supported!");
+ }
+
+ Future<Void> closeInternal()
+ {
+ return workerExecutor == null
+ ? Future.succeededFuture()
+ : workerExecutor.close();
+ }
+
+ /**
+ * {@link ServiceTaskExecutorPool} is used for executing tasks that are short lived and not expected to block for
+ * too long, therefore will free up resources more quickly
+ */
+ static class ServiceTaskExecutorPool extends TaskExecutorPool
+ {
+ private final ResourceMetrics metrics;
+
+ ServiceTaskExecutorPool(Vertx vertx, WorkerPoolConfiguration config, ResourceMetrics metrics)
+ {
+ super(vertx, config);
+ this.metrics = metrics;
+ }
+
+ @Override
+ protected void recordTimeTaken(long duration)
+ {
+ if (metrics == null)
Review Comment:
will `metrics` ever be null? it doesn't seem like we do null checks for other metrics anywhere else in the code
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableMetrics.java:
##########
@@ -71,15 +79,21 @@ public UploadSSTableComponentMetrics(MetricRegistry metricRegistry, String sstab
NamedMetric.Tag componentTag = NamedMetric.Tag.of("component", sstableComponent);
rateLimitedCalls
- = NamedMetric.builder(metricRegistry::meter)
+ = NamedMetric.builder(name -> metricRegistry.gauge(name, () -> new DefaultSettableGauge<>(0)))
Review Comment:
I'm not sure if rate limited calls are tied to a specific component. Is there a reason it matters to make it component specific?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561422062
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceHealthMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks health related metrics of a Cassandra instance maintained by Sidecar.
+ */
+public class InstanceHealthMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".health";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<Meter> jmxDown;
+ public final NamedMetric<Meter> nativeDown;
Review Comment:
Meters also give count of instances. But it is more expensive since it captures moving averages. Will change these to Gauges
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561838523
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/SSTableImportMetrics.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.instance;
+
+import java.util.Objects;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * Tracks metrics during SSTable import operation done in Cassandra instance
+ */
+public class SSTableImportMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".SSTableImport";
+ protected final MetricRegistry metricRegistry;
+ public final NamedMetric<DefaultSettableGauge<Integer>> cassandraUnavailable;
Review Comment:
I missed the unavailable exceptions thrown there. Will add there
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1561838832
##########
src/main/java/org/apache/cassandra/sidecar/utils/SSTableImporter.java:
##########
@@ -159,16 +159,45 @@ protected ImportQueue initializeQueue(String key)
*/
private void processPendingImports(Long timerId)
{
- for (ImportQueue queue : importQueuePerHost.values())
+ for (Map.Entry<String, ImportQueue> entry : importQueuePerHost.entrySet())
{
+ ImportQueue queue = entry.getValue();
if (!queue.isEmpty())
{
+ recordPendingImports(hostFromKey(entry.getKey()), entry.getValue().size());
executorPools.internal()
.executeBlocking(p -> maybeDrainImportQueue(queue));
}
}
}
+ private void recordPendingImports(String host, int pendingImports)
+ {
+ if (host != null)
+ {
+ metadataFetcher.instance(host)
+ .metrics()
+ .sstableImport().pendingImports.metric.setValue(pendingImports);
+ }
+ }
+
+ /**
+ * Host is extracted from key for publishing Cassandra instance specific metrics related to {@link ImportQueue}
+ *
+ * @param key a key for one of the import queues
+ * @return host for which the import queue is maintained
+ */
+ protected String hostFromKey(String key)
+ {
+ String[] keyParts = key.split("\\$");
Review Comment:
As discussed will capture the metric while draining the queue
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111#discussion_r1562845635
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/VertxMetricsConfigurationImpl.java:
##########
@@ -32,34 +29,28 @@ public class VertxMetricsConfigurationImpl implements VertxMetricsConfiguration
public static final boolean DEFAULT_ENABLED = true;
public static final boolean DEFAULT_EXPOSE_VIA_JMX = false;
public static final String DEFAULT_JMX_DOMAIN_NAME = "sidecar.vertx.jmx_domain";
- public static final List<String> DEFAULT_MONITORED_SERVER_ROUTE_REGEXES = Collections.singletonList("/api/v1/.*");
Review Comment:
Discussed with Fracisco, we will use the exclude list instead of configuring monitored server routes
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRASC-117 Record existing and additional metrics with dropwizard [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh merged PR #111:
URL: https://github.com/apache/cassandra-sidecar/pull/111
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org