yifan-c commented on code in PR #102:
URL: https://github.com/apache/cassandra-sidecar/pull/102#discussion_r1533089222
##########
src/test/java/org/apache/cassandra/sidecar/tasks/HealthCheckPeriodicTaskTest.java:
##########
@@ -136,7 +160,11 @@ void
testDelegateExceptionDoesntPreventChecksOnOtherInstances(VertxTestContext c
when(mockInstancesConfig.instances()).thenReturn(mockInstanceMetadata);
Promise<Void> promise = Promise.promise();
healthCheck.execute(promise);
- promise.future().onComplete(context.succeedingThenComplete());
+ promise.future().onComplete(context.failing(v -> {
Review Comment:
The assertion change makes sense. How was the test passing before? hmm...
##########
src/main/java/org/apache/cassandra/sidecar/metrics/NamedMetric.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import com.codahale.metrics.Metric;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+
+/**
+ * {@link NamedMetric} is for creating {@link Metric} with a structured name.
Metric name should contain domain it
+ * is captured for and optional tags for additional context.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+public class NamedMetric<T extends Metric>
+{
+ public final String canonicalName;
+ public final T metric;
+
+ private NamedMetric(Builder<T> builder)
+ {
+ this.canonicalName = builder.makeFullName();
+ this.metric = builder.register(canonicalName);
+ }
+
+ public static <T extends Metric> Builder<T> builder(Function<String, T>
metricCreator)
+ {
+ return new Builder<>(metricCreator);
+ }
+
+ /**
+ * {@link NamedMetric} builder static inner class.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+ public static class Builder<T extends Metric> implements
DataObjectBuilder<Builder<T>, NamedMetric<T>>
+ {
+ private final Function<String, T> metricCreator;
+ private final List<Tag> tags = new ArrayList<>();
+ private String domain;
+ private String name;
+
+ public Builder(Function<String, T> metricCreator)
+ {
+ this.metricCreator = metricCreator;
+ }
+
+ /**
+ * Sets {@code domain} of metric and returns a reference to this
builder to enable method chaining.
+ *
+ * @param domain domain metric is part of
+ * @return a reference to this Builder
+ */
Review Comment:
" and returns a reference to this builder to enable method chaining." is not
helpful for the same reason. I am aware that other existing builder methods
have the same style javadoc. So feel free to ignore this nit.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/NamedMetric.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import com.codahale.metrics.Metric;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+
+/**
+ * {@link NamedMetric} is for creating {@link Metric} with a structured name.
Metric name should contain domain it
+ * is captured for and optional tags for additional context.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+public class NamedMetric<T extends Metric>
+{
+ public final String canonicalName;
+ public final T metric;
Review Comment:
can you add a test case for `NamedMetric` to assert the retrieved metric
with `canonicalName` is the same metric?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetricRegistry.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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 com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.SharedMetricRegistries;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import org.apache.cassandra.sidecar.cluster.InstancesConfig;
+import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata;
+
+/**
+ * A dropwizard {@link MetricRegistry} associated with a specific Cassandra
instance.
+ */
+public class InstanceMetricRegistry extends MetricRegistry
Review Comment:
once `InstanceMetrics` is part of `InstanceMetadata`, this class can be
delete too.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetrics.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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;
+
+/**
+ * {@link InstanceMetrics} tracks metrics related to a Cassandra instance that
Sidecar maintains.
+ */
+public interface InstanceMetrics
+{
+ String INSTANCE_PREFIX = "sidecar.instance";
Review Comment:
tbh, the meaning of the word `instance` is vague. W/o looking at the code
comment, one can be easily confused by the string "sidecar.instance",
especially they just look at the metrics name and have no familiarity with the
code base. It reads as sidecar instance.
How about changing it to `sidecar.cass_instance`
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetrics.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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;
+
+/**
+ * {@link InstanceMetrics} tracks metrics related to a Cassandra instance that
Sidecar maintains.
+ */
+public interface InstanceMetrics
+{
+ String INSTANCE_PREFIX = "sidecar.instance";
+
+ /**
+ * @return resource metrics tracked for instance
+ */
+ InstanceResourceMetrics resource();
+
+ /**
+ * @param component SSTable component such as data
+ * @return metrics that are tracked during streaming of provided SSTable
component
+ */
+ StreamSSTableComponentMetrics forStreamComponent(String component);
+
+ /**
+ * @param component SSTable component such as data
+ * @return metrics that are tracked during upload of provided SSTable
component
+ */
+ UploadSSTableComponentMetrics forUploadComponent(String component);
Review Comment:
same here. `UploadSSTableComponentMetrics` should contain the metrics of
each component.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetricProviderImpl.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.cassandra.sidecar.cluster.InstancesConfig;
+import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata;
+
+/**
+ * Provider for getting instance specific metrics given Cassandra instance id
or host name.
+ */
+public class InstanceMetricProviderImpl implements InstanceMetricProvider
Review Comment:
it can be deleted.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableComponentMetrics.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link UploadSSTableComponentMetrics} tracks metrics captured during upload
of SSTable component into Sidecar.
+ */
+public class UploadSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".upload";
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ public final String sstableComponent;
+ public final Meter rateLimitedCalls;
+ public final Meter diskUsageHighErrors;
+
+ public UploadSSTableComponentMetrics(InstanceMetricRegistry
instanceMetricRegistry,
+ String sstableComponent)
+ {
+ this.instanceMetricRegistry
+ = Objects.requireNonNull(instanceMetricRegistry, "Metric registry can
not be null");
+ this.sstableComponent
+ = Objects.requireNonNull(sstableComponent, "SSTable component required
for component specific metrics capture");
+ if (sstableComponent.isEmpty())
+ {
+ throw new IllegalArgumentException("SSTableComponent required for
component specific metrics capture");
+ }
+
+ NamedMetric.Tag componentTag = NamedMetric.Tag.of("component",
sstableComponent);
+
+ rateLimitedCalls
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("rate_limited_calls_429")
Review Comment:
"throttled_429"?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/StreamSSTableComponentMetrics.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.Timer;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link StreamSSTableComponentMetrics} tracks metrics captured during
streaming of SSTable component from Sidecar.
+ */
+public class StreamSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".stream";
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ public final String sstableComponent;
+ public final Meter rateLimitedCalls;
+ public final Timer timeTakenForSendFile;
+ public final Timer waitTimeSent;
+
+ public StreamSSTableComponentMetrics(InstanceMetricRegistry
instanceMetricRegistry,
+ String sstableComponent)
+ {
+ this.instanceMetricRegistry =
Objects.requireNonNull(instanceMetricRegistry, "Metric registry can not be
null");
+ this.sstableComponent
+ = Objects.requireNonNull(sstableComponent, "SSTable component required
for component specific metrics capture");
+ if (sstableComponent.isEmpty())
+ {
+ throw new IllegalArgumentException("SSTableComponent required for
component specific metrics capture");
+ }
+
+ NamedMetric.Tag componentTag = NamedMetric.Tag.of("component",
sstableComponent);
+
+ rateLimitedCalls
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("rate_limited_calls_429")
Review Comment:
"throttled_429"?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/StreamSSTableComponentMetrics.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.Timer;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link StreamSSTableComponentMetrics} tracks metrics captured during
streaming of SSTable component from Sidecar.
+ */
+public class StreamSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".stream";
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ public final String sstableComponent;
+ public final Meter rateLimitedCalls;
+ public final Timer timeTakenForSendFile;
+ public final Timer waitTimeSent;
+
+ public StreamSSTableComponentMetrics(InstanceMetricRegistry
instanceMetricRegistry,
+ String sstableComponent)
+ {
+ this.instanceMetricRegistry =
Objects.requireNonNull(instanceMetricRegistry, "Metric registry can not be
null");
+ this.sstableComponent
+ = Objects.requireNonNull(sstableComponent, "SSTable component required
for component specific metrics capture");
+ if (sstableComponent.isEmpty())
+ {
+ throw new IllegalArgumentException("SSTableComponent required for
component specific metrics capture");
+ }
+
+ NamedMetric.Tag componentTag = NamedMetric.Tag.of("component",
sstableComponent);
+
+ rateLimitedCalls
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("rate_limited_calls_429")
+ .addTag(componentTag)
+ .build().metric;
+ timeTakenForSendFile
+ = NamedMetric.builder(instanceMetricRegistry::timer)
+ .withDomain(DOMAIN)
+ .withName("time_taken_for_sendfile")
+ .addTag(componentTag)
+ .build().metric;
+ waitTimeSent
Review Comment:
is it really needed to report the retryAfter duration values that are sent
to client?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetricProvider.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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;
+
+/**
+ * Provider for getting instance specific metrics given instance id or host.
+ */
+public interface InstanceMetricProvider
Review Comment:
how about removing this interface and its implementation, and provide
`InstanceMetrics` from `InstanceMetadata`? i.e. adding a new interface method
in `InstanceMetadata` that return `InstanceMetrics`
##########
src/main/java/org/apache/cassandra/sidecar/metrics/ServerMetrics.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/**
+ * Tracks metrics related to server initialization, periodic tasks related to
server.
+ */
+@Singleton
+public class ServerMetrics
+{
+ public static final String DOMAIN = "sidecar.server";
+ protected final MetricRegistry metricRegistry;
+ public final DefaultSettableGauge<Integer> cassandraInstancesUp;
+ public final DefaultSettableGauge<Integer> cassandraInstancesDown;
Review Comment:
It is sufficient to only keep the reference of metrics for collection
purpose. However, the metric names are lost and seemingly should be kept.
Admitted that the metric names are not used anywhere. So I am on the fence.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/NamedMetric.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import com.codahale.metrics.Metric;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+
+/**
+ * {@link NamedMetric} is for creating {@link Metric} with a structured name.
Metric name should contain domain it
+ * is captured for and optional tags for additional context.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+public class NamedMetric<T extends Metric>
+{
+ public final String canonicalName;
+ public final T metric;
+
+ private NamedMetric(Builder<T> builder)
+ {
+ this.canonicalName = builder.makeFullName();
+ this.metric = builder.register(canonicalName);
+ }
+
+ public static <T extends Metric> Builder<T> builder(Function<String, T>
metricCreator)
+ {
+ return new Builder<>(metricCreator);
+ }
+
+ /**
+ * {@link NamedMetric} builder static inner class.
Review Comment:
nit: the sentence is not helpful to mention that the builder is a static
inner class. It of course is! Let's just say `Builder for {@link NamedMetric}`.
Wdyt?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/NamedMetric.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import com.codahale.metrics.Metric;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+
+/**
+ * {@link NamedMetric} is for creating {@link Metric} with a structured name.
Metric name should contain domain it
+ * is captured for and optional tags for additional context.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+public class NamedMetric<T extends Metric>
+{
+ public final String canonicalName;
+ public final T metric;
+
+ private NamedMetric(Builder<T> builder)
+ {
+ this.canonicalName = builder.makeFullName();
+ this.metric = builder.register(canonicalName);
+ }
+
+ public static <T extends Metric> Builder<T> builder(Function<String, T>
metricCreator)
+ {
+ return new Builder<>(metricCreator);
+ }
+
+ /**
+ * {@link NamedMetric} builder static inner class.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
+ */
+ public static class Builder<T extends Metric> implements
DataObjectBuilder<Builder<T>, NamedMetric<T>>
Review Comment:
Can you change the builder to this (meaning it builds a metric directly), if
metric name is not going to be discarded?
```suggestion
public static class Builder<T extends Metric> implements
DataObjectBuilder<Builder<T>, T>
```
##########
src/main/java/org/apache/cassandra/sidecar/metrics/NamedMetric.java:
##########
@@ -0,0 +1,171 @@
+/*
+ * 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.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+
+import com.codahale.metrics.Metric;
+import org.apache.cassandra.sidecar.common.DataObjectBuilder;
+
+/**
+ * {@link NamedMetric} is for creating {@link Metric} with a structured name.
Metric name should contain domain it
+ * is captured for and optional tags for additional context.
+ *
+ * @param <T> {@link NamedMetric} can be created for any {@link Metric}
type. T represents the Metric type
+ * {@link NamedMetric} represents
Review Comment:
This can be simplified to the below.
```suggestion
* @param <T> Metric type
```
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableComponentMetrics.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link UploadSSTableComponentMetrics} tracks metrics captured during upload
of SSTable component into Sidecar.
+ */
+public class UploadSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".upload";
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ public final String sstableComponent;
+ public final Meter rateLimitedCalls;
+ public final Meter diskUsageHighErrors;
+
+ public UploadSSTableComponentMetrics(InstanceMetricRegistry
instanceMetricRegistry,
+ String sstableComponent)
+ {
+ this.instanceMetricRegistry
+ = Objects.requireNonNull(instanceMetricRegistry, "Metric registry can
not be null");
+ this.sstableComponent
+ = Objects.requireNonNull(sstableComponent, "SSTable component required
for component specific metrics capture");
+ if (sstableComponent.isEmpty())
+ {
+ throw new IllegalArgumentException("SSTableComponent required for
component specific metrics capture");
+ }
+
+ NamedMetric.Tag componentTag = NamedMetric.Tag.of("component",
sstableComponent);
+
+ rateLimitedCalls
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("rate_limited_calls_429")
+ .addTag(componentTag)
+ .build().metric;
+ diskUsageHighErrors
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("disk_usage_high_errors")
Review Comment:
it is the same as insufficient_staging_space. can you remove this metric?
and collect with that one instead.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetricsImpl.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Tracks metrics related to a Cassandra instance that Sidecar maintains.
+ */
+public class InstanceMetricsImpl implements InstanceMetrics
+{
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ protected final Map<String, StreamSSTableComponentMetrics>
streamComponentMetrics = new ConcurrentHashMap<>();
+ protected final Map<String, UploadSSTableComponentMetrics>
uploadComponentMetrics = new ConcurrentHashMap<>();
Review Comment:
They do not belong in the `InstanceMetricsImpl`
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceMetrics.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * 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;
+
+/**
+ * {@link InstanceMetrics} tracks metrics related to a Cassandra instance that
Sidecar maintains.
+ */
+public interface InstanceMetrics
+{
+ String INSTANCE_PREFIX = "sidecar.instance";
+
+ /**
+ * @return resource metrics tracked for instance
+ */
+ InstanceResourceMetrics resource();
+
+ /**
+ * @param component SSTable component such as data
+ * @return metrics that are tracked during streaming of provided SSTable
component
+ */
+ StreamSSTableComponentMetrics forStreamComponent(String component);
Review Comment:
`StreamSSTableComponentMetrics` should contain the metrics of each
component. The input `component` should be removed from the interface method.
Otherwise, the implementation have to maintain metrics per component, which is
not the right place.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/InstanceResourceMetrics.java:
##########
@@ -0,0 +1,48 @@
+/*
+ * 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 org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link InstanceResourceMetrics} contains metrics to track resource usage of
a Cassandra instance maintained
+ * by Sidecar.
+ */
+public class InstanceResourceMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".resource";
+ protected final InstanceMetricRegistry metricRegistry;
+ public final Meter insufficientStorageErrors;
+
+ public InstanceResourceMetrics(InstanceMetricRegistry metricRegistry)
+ {
+ this.metricRegistry = Objects.requireNonNull(metricRegistry, "Metric
registry can not be null");
+
+ insufficientStorageErrors
+ = NamedMetric.builder(metricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("insufficient_storage_errors")
Review Comment:
the name has to be more specific to avoid misleading. The metric is
reporting on insufficient space for staging.
my suggestion of new name is `insufficient_staging_space`
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/StreamSSTableComponentMetrics.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.Timer;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link StreamSSTableComponentMetrics} tracks metrics captured during
streaming of SSTable component from Sidecar.
+ */
+public class StreamSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".stream";
+ protected final InstanceMetricRegistry instanceMetricRegistry;
+ public final String sstableComponent;
+ public final Meter rateLimitedCalls;
+ public final Timer timeTakenForSendFile;
+ public final Timer waitTimeSent;
+
+ public StreamSSTableComponentMetrics(InstanceMetricRegistry
instanceMetricRegistry,
+ String sstableComponent)
+ {
+ this.instanceMetricRegistry =
Objects.requireNonNull(instanceMetricRegistry, "Metric registry can not be
null");
+ this.sstableComponent
+ = Objects.requireNonNull(sstableComponent, "SSTable component required
for component specific metrics capture");
+ if (sstableComponent.isEmpty())
+ {
+ throw new IllegalArgumentException("SSTableComponent required for
component specific metrics capture");
+ }
+
+ NamedMetric.Tag componentTag = NamedMetric.Tag.of("component",
sstableComponent);
+
+ rateLimitedCalls
+ = NamedMetric.builder(instanceMetricRegistry::meter)
+ .withDomain(DOMAIN)
+ .withName("rate_limited_calls_429")
+ .addTag(componentTag)
+ .build().metric;
+ timeTakenForSendFile
+ = NamedMetric.builder(instanceMetricRegistry::timer)
+ .withDomain(DOMAIN)
+ .withName("time_taken_for_sendfile")
Review Comment:
"sendfile_latency"?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/StreamSSTableComponentMetrics.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.Timer;
+import org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link StreamSSTableComponentMetrics} tracks metrics captured during
streaming of SSTable component from Sidecar.
+ */
+public class StreamSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".stream";
Review Comment:
"stream" is too general. and in the future, we could stream other things.
Let's put "stream_sstable"
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/UploadSSTableComponentMetrics.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 org.apache.cassandra.sidecar.metrics.NamedMetric;
+
+import static
org.apache.cassandra.sidecar.metrics.instance.InstanceMetrics.INSTANCE_PREFIX;
+
+/**
+ * {@link UploadSSTableComponentMetrics} tracks metrics captured during upload
of SSTable component into Sidecar.
+ */
+public class UploadSSTableComponentMetrics
+{
+ public static final String DOMAIN = INSTANCE_PREFIX + ".upload";
Review Comment:
"upload_sstable"
##########
src/main/java/org/apache/cassandra/sidecar/metrics/instance/MetricRegistryNameFactory.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * 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 com.google.inject.Inject;
+import com.google.inject.Singleton;
+import org.apache.cassandra.sidecar.config.SidecarConfiguration;
+
+/**
+ * Provides registry name for instance specific registry {@link
InstanceMetricRegistry}.
+ */
+@Singleton
+public class MetricRegistryNameFactory
Review Comment:
This class look unnecessary. It can be a private method in
`InstanceMetadata`, when exposing metrics from 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
