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/03/28 20:27:05 UTC
[PR] CASSANDRASC-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
sarankk opened a new pull request, #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109
(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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560055850
##########
src/main/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistry.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.codahale.metrics.Timer;
+import io.vertx.core.impl.ConcurrentHashSet;
+
+/**
+ * Allows filtering of metrics based on configured allow list. Metrics are filtered out before registering them.
+ */
+public class FilteringMetricRegistry extends MetricRegistry
+{
+ private static final NoopMetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry(); // supplies no-op metrics
+ private final Predicate<String> isAllowed;
Review Comment:
I don't feel the variable name fully conveys what the variable is supposed to do. Maybe another name is possible too?
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560043715
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
Review Comment:
This is useful in general, if people want to pass filters and create the class, instead of always wanting `SidecarConfiguration` for creating a factor.
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#issuecomment-2048196423
Thanks Yifan!, addressed feedback
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560046404
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
+ try
+ {
+ metricRegistry = new FilteringMetricRegistry(this::isAllowed);
+ }
+ finally
+ {
+ readWriteLock.readLock().unlock();
+ }
+ SharedMetricRegistries.add(name, metricRegistry);
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ /**
+ * Check if the metric is allowed to register
+ * The evaluation order is inclusions first, then exclusions. In other words,
+ * a metric name is allowed if it is in the inclusions, but not in the exclusions.
+ * <p>
+ * Note that an empty inclusions means including all
+ *
+ * @param name metric name
+ * @return true if allowed; false otherwise
+ */
+ private boolean isAllowed(String name)
+ {
+ boolean included = inclusions.isEmpty() || inclusions.stream().anyMatch(filter -> filter.matches(name));
+ boolean excluded = exclusions.stream().anyMatch(filter -> filter.matches(name));
Review Comment:
In an example where configuration is
```
include:
- type: "regex"
value: "vertx.*"
exclude:
- type: "regex"
value: "vertx.eventbus.*"
```
We want to include all vert.x metrics except the event bus metrics. In that case, both included and excluded will be true. So it not necessarily a configuration error.
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560058656
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
Review Comment:
fair, makes sense
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560041529
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
Review Comment:
Yes discussed, without reconfiguring filters we don't need a lock at all. So removing this
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560054031
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricFilter.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.regex.Pattern;
+
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Filter for deciding whether a metric should be captured
+ */
+public abstract class MetricFilter
+{
+ public abstract boolean matches(String name);
+
+ public static List<MetricFilter> parse(List<MetricsFilteringConfiguration> filterConfigurations)
+ {
+ List<MetricFilter> filters = new ArrayList<>();
+ for (MetricsFilteringConfiguration filterConfiguration : filterConfigurations)
+ {
+ if (filterConfiguration.type().equalsIgnoreCase("regex"))
+ {
+ filters.add(new Regex(filterConfiguration.value()));
+ }
+ else if (filterConfiguration.type().equalsIgnoreCase("equals"))
+ {
+ filters.add(new Equals(filterConfiguration.value()));
+ }
+ }
+ return filters;
+ }
+
+ /**
+ * Metric name based {@link MetricFilter} that checks if a metric name matches provided regex pattern
+ */
+ public static class Regex extends MetricFilter
+ {
+ private final Pattern pattern;
+
+ public Regex(String regex)
+ {
+ Objects.requireNonNull(regex);
+ this.pattern = Pattern.compile(regex);
+ }
+
+ public boolean matches(String name)
Review Comment:
can you add override annotations ?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricFilter.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.regex.Pattern;
+
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Filter for deciding whether a metric should be captured
+ */
+public abstract class MetricFilter
+{
+ public abstract boolean matches(String name);
Review Comment:
javadocs?
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560071614
##########
src/test/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistryTest.java:
##########
@@ -0,0 +1,216 @@
+/*
+ * 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.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.regex.Pattern;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import io.vertx.core.Vertx;
+import io.vertx.ext.dropwizard.ThroughputMeter;
+import io.vertx.junit5.Checkpoint;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.server.MainModule;
+import org.apache.cassandra.sidecar.server.Server;
+import org.apache.cassandra.sidecar.server.SidecarServerEvents;
+
+import static org.apache.cassandra.sidecar.common.ResourceUtils.writeResourceToPath;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+/**
+ * Test for filtering of metrics
+ */
+@ExtendWith(VertxExtension.class)
+public class FilteringMetricRegistryTest
+{
+ private static final MetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry();
+ @TempDir
+ private Path confPath;
+
+ @Test
+ void testNoopInstanceRetrieved()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.timer("testMetricTimer")).isSameAs(NO_OP_METRIC_REGISTRY.timer("any"));
+ assertThat(metricRegistry.meter("testMetricMeter")).isSameAs(NO_OP_METRIC_REGISTRY.meter("any"));
+ assertThat(metricRegistry.counter("testMetricCounter")).isSameAs(NO_OP_METRIC_REGISTRY.counter("any"));
+ assertThat(metricRegistry.histogram("testMetricHistogram")).isSameAs(NO_OP_METRIC_REGISTRY.histogram("any"));
+
+ metricRegistry.register("testMetricThroughputMeter", new ThroughputMeter());
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricThroughputMeter")).isFalse();
+ }
+
+ @Test
+ void testDuplicateMetricsNotAllowed()
+ {
+ MetricRegistry metricRegistry = new MetricRegistry();
+ assertThat(metricRegistry.timer("testMetric")).isNotNull();
+ assertThatThrownBy(() -> metricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.emptyList());
+ FilteringMetricRegistry filteringMetricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ filteringMetricRegistry.timer("testMetric");
+ assertThatThrownBy(() -> filteringMetricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ void testGaugeMetricExclusion()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.gauge("testMetricGauge", () -> new DefaultSettableGauge<>(0L)))
+ .isInstanceOf(DefaultSettableGauge.class);
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricGauge")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeLong", new DefaultSettableGauge<>(0L));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeLong")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeDouble", new DefaultSettableGauge<>(0d));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeDouble")).isFalse();
+ }
+
+ @Test
+ void testOneMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("vertx.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(exactFilter),
+ Collections.singletonList(regexFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testMultipleMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Arrays.asList(exactFilter, regexFilter),
+ Collections.emptyList());
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testExcludingEqualsMetricFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExcludingRegexMetricFilter()
+ {
+ MetricFilter.Regex vertxFilter = new MetricFilter.Regex("vertx.*");
+ MetricFilter.Regex sidecarFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryProvider = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(sidecarFilter),
+ Collections.singletonList(vertxFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryProvider.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ metricRegistry.timer("vertx.eventbus.message_transfer_time");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("vertx.eventbus.message_transfer_time")).isFalse();
+ }
+
+ @Test
+ void testMultipleMatchingFilterWithOneExclude()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(regexFilter),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExclusionsWithServer(VertxTestContext context)
+ {
+ ClassLoader classLoader = FilteringMetricRegistryTest.class.getClassLoader();
+ Path yamlPath = writeResourceToPath(classLoader, confPath, "config/sidecar_metrics.yaml");
+ Injector injector = Guice.createInjector(new MainModule(yamlPath));
+ Server server = injector.getInstance(Server.class);
+ Vertx vertx = injector.getInstance(Vertx.class);
+
+ Checkpoint serverStarted = context.checkpoint();
+ Checkpoint waitUntilCheck = context.checkpoint();
+
+ vertx.eventBus().localConsumer(SidecarServerEvents.ON_SERVER_START.address(), message -> serverStarted.flag());
+
+ server.start()
+ .onFailure(context::failNow)
+ .onSuccess(v -> {
+ MetricRegistryFactory registryFactory = injector.getInstance(MetricRegistryFactory.class);
+ Pattern excludedPattern = Pattern.compile("vertx.eventbus.*");
+ FilteringMetricRegistry globalRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+ assertThat(globalRegistry.getIncludedMetrics().size()).isGreaterThanOrEqualTo(1);
+ assertThat(globalRegistry.getIncludedMetrics()
+ .keySet()
+ .stream()
+ .noneMatch(key -> excludedPattern.matcher(key).matches()))
+ .isTrue();
Review Comment:
I already use `noneMatch` are you suggesting removing `isTrue`?
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560126097
##########
src/test/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistryTest.java:
##########
@@ -0,0 +1,216 @@
+/*
+ * 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.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.regex.Pattern;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import io.vertx.core.Vertx;
+import io.vertx.ext.dropwizard.ThroughputMeter;
+import io.vertx.junit5.Checkpoint;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.server.MainModule;
+import org.apache.cassandra.sidecar.server.Server;
+import org.apache.cassandra.sidecar.server.SidecarServerEvents;
+
+import static org.apache.cassandra.sidecar.common.ResourceUtils.writeResourceToPath;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+/**
+ * Test for filtering of metrics
+ */
+@ExtendWith(VertxExtension.class)
+public class FilteringMetricRegistryTest
+{
+ private static final MetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry();
+ @TempDir
+ private Path confPath;
+
+ @Test
+ void testNoopInstanceRetrieved()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.timer("testMetricTimer")).isSameAs(NO_OP_METRIC_REGISTRY.timer("any"));
+ assertThat(metricRegistry.meter("testMetricMeter")).isSameAs(NO_OP_METRIC_REGISTRY.meter("any"));
+ assertThat(metricRegistry.counter("testMetricCounter")).isSameAs(NO_OP_METRIC_REGISTRY.counter("any"));
+ assertThat(metricRegistry.histogram("testMetricHistogram")).isSameAs(NO_OP_METRIC_REGISTRY.histogram("any"));
+
+ metricRegistry.register("testMetricThroughputMeter", new ThroughputMeter());
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricThroughputMeter")).isFalse();
+ }
+
+ @Test
+ void testDuplicateMetricsNotAllowed()
+ {
+ MetricRegistry metricRegistry = new MetricRegistry();
+ assertThat(metricRegistry.timer("testMetric")).isNotNull();
+ assertThatThrownBy(() -> metricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.emptyList());
+ FilteringMetricRegistry filteringMetricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ filteringMetricRegistry.timer("testMetric");
+ assertThatThrownBy(() -> filteringMetricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ void testGaugeMetricExclusion()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.gauge("testMetricGauge", () -> new DefaultSettableGauge<>(0L)))
+ .isInstanceOf(DefaultSettableGauge.class);
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricGauge")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeLong", new DefaultSettableGauge<>(0L));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeLong")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeDouble", new DefaultSettableGauge<>(0d));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeDouble")).isFalse();
+ }
+
+ @Test
+ void testOneMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("vertx.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(exactFilter),
+ Collections.singletonList(regexFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testMultipleMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Arrays.asList(exactFilter, regexFilter),
+ Collections.emptyList());
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testExcludingEqualsMetricFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExcludingRegexMetricFilter()
+ {
+ MetricFilter.Regex vertxFilter = new MetricFilter.Regex("vertx.*");
+ MetricFilter.Regex sidecarFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryProvider = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(sidecarFilter),
+ Collections.singletonList(vertxFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryProvider.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ metricRegistry.timer("vertx.eventbus.message_transfer_time");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("vertx.eventbus.message_transfer_time")).isFalse();
+ }
+
+ @Test
+ void testMultipleMatchingFilterWithOneExclude()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(regexFilter),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExclusionsWithServer(VertxTestContext context)
+ {
+ ClassLoader classLoader = FilteringMetricRegistryTest.class.getClassLoader();
+ Path yamlPath = writeResourceToPath(classLoader, confPath, "config/sidecar_metrics.yaml");
+ Injector injector = Guice.createInjector(new MainModule(yamlPath));
+ Server server = injector.getInstance(Server.class);
+ Vertx vertx = injector.getInstance(Vertx.class);
+
+ Checkpoint serverStarted = context.checkpoint();
+ Checkpoint waitUntilCheck = context.checkpoint();
+
+ vertx.eventBus().localConsumer(SidecarServerEvents.ON_SERVER_START.address(), message -> serverStarted.flag());
+
+ server.start()
+ .onFailure(context::failNow)
+ .onSuccess(v -> {
+ MetricRegistryFactory registryFactory = injector.getInstance(MetricRegistryFactory.class);
+ Pattern excludedPattern = Pattern.compile("vertx.eventbus.*");
+ FilteringMetricRegistry globalRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+ assertThat(globalRegistry.getIncludedMetrics().size()).isGreaterThanOrEqualTo(1);
+ assertThat(globalRegistry.getIncludedMetrics()
+ .keySet()
+ .stream()
+ .noneMatch(key -> excludedPattern.matcher(key).matches()))
+ .isTrue();
Review Comment:
Addressed this
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#issuecomment-2048440508
Thanks @frankgh for the comments. Addressed them and your comment about remembering the predicate results with a `CachedPredicate`
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1556206283
##########
src/main/dist/conf/sidecar.yaml:
##########
@@ -150,6 +150,12 @@ metrics:
jmx_domain_name: sidecar.vertx.jmx_domain
monitored_server_route_regexes: # regex list to match server routes
- /api/v1/.*
+ include:
+ - type: "regex" # other possible value is "equals"
Review Comment:
other possible ~value~ _type_ is "equals"
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560061853
##########
src/test/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistryTest.java:
##########
@@ -0,0 +1,216 @@
+/*
+ * 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.nio.file.Path;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.UUID;
+import java.util.regex.Pattern;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+import com.codahale.metrics.DefaultSettableGauge;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import io.vertx.core.Vertx;
+import io.vertx.ext.dropwizard.ThroughputMeter;
+import io.vertx.junit5.Checkpoint;
+import io.vertx.junit5.VertxExtension;
+import io.vertx.junit5.VertxTestContext;
+import org.apache.cassandra.sidecar.server.MainModule;
+import org.apache.cassandra.sidecar.server.Server;
+import org.apache.cassandra.sidecar.server.SidecarServerEvents;
+
+import static org.apache.cassandra.sidecar.common.ResourceUtils.writeResourceToPath;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
+
+/**
+ * Test for filtering of metrics
+ */
+@ExtendWith(VertxExtension.class)
+public class FilteringMetricRegistryTest
+{
+ private static final MetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry();
+ @TempDir
+ private Path confPath;
+
+ @Test
+ void testNoopInstanceRetrieved()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.timer("testMetricTimer")).isSameAs(NO_OP_METRIC_REGISTRY.timer("any"));
+ assertThat(metricRegistry.meter("testMetricMeter")).isSameAs(NO_OP_METRIC_REGISTRY.meter("any"));
+ assertThat(metricRegistry.counter("testMetricCounter")).isSameAs(NO_OP_METRIC_REGISTRY.counter("any"));
+ assertThat(metricRegistry.histogram("testMetricHistogram")).isSameAs(NO_OP_METRIC_REGISTRY.histogram("any"));
+
+ metricRegistry.register("testMetricThroughputMeter", new ThroughputMeter());
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricThroughputMeter")).isFalse();
+ }
+
+ @Test
+ void testDuplicateMetricsNotAllowed()
+ {
+ MetricRegistry metricRegistry = new MetricRegistry();
+ assertThat(metricRegistry.timer("testMetric")).isNotNull();
+ assertThatThrownBy(() -> metricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.emptyList());
+ FilteringMetricRegistry filteringMetricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ filteringMetricRegistry.timer("testMetric");
+ assertThatThrownBy(() -> filteringMetricRegistry.meter("testMetric"))
+ .isInstanceOf(IllegalArgumentException.class);
+ }
+
+ @Test
+ void testGaugeMetricExclusion()
+ {
+ MetricFilter.Regex testFilter = new MetricFilter.Regex("testMetric.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(testFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ assertThat(metricRegistry.gauge("testMetricGauge", () -> new DefaultSettableGauge<>(0L)))
+ .isInstanceOf(DefaultSettableGauge.class);
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricGauge")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeLong", new DefaultSettableGauge<>(0L));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeLong")).isFalse();
+
+ metricRegistry.register("testMetricDefaultSettableGaugeDouble", new DefaultSettableGauge<>(0d));
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("testMetricDefaultSettableGaugeDouble")).isFalse();
+ }
+
+ @Test
+ void testOneMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("vertx.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(exactFilter),
+ Collections.singletonList(regexFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testMultipleMatchingFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Arrays.asList(exactFilter, regexFilter),
+ Collections.emptyList());
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ }
+
+ @Test
+ void testExcludingEqualsMetricFilter()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.emptyList(),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExcludingRegexMetricFilter()
+ {
+ MetricFilter.Regex vertxFilter = new MetricFilter.Regex("vertx.*");
+ MetricFilter.Regex sidecarFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryProvider = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(sidecarFilter),
+ Collections.singletonList(vertxFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryProvider.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getMetrics().containsKey("sidecar.metric.exact")).isTrue();
+ metricRegistry.timer("vertx.eventbus.message_transfer_time");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("vertx.eventbus.message_transfer_time")).isFalse();
+ }
+
+ @Test
+ void testMultipleMatchingFilterWithOneExclude()
+ {
+ MetricFilter.Equals exactFilter = new MetricFilter.Equals("sidecar.metric.exact");
+ MetricFilter.Regex regexFilter = new MetricFilter.Regex("sidecar.*");
+ MetricRegistryFactory registryFactory = new MetricRegistryFactory("cassandra_sidecar_" + UUID.randomUUID(),
+ Collections.singletonList(regexFilter),
+ Collections.singletonList(exactFilter));
+ FilteringMetricRegistry metricRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+
+ metricRegistry.meter("sidecar.metric.exact");
+ assertThat(metricRegistry.getIncludedMetrics().containsKey("sidecar.metric.exact")).isFalse();
+ }
+
+ @Test
+ void testExclusionsWithServer(VertxTestContext context)
+ {
+ ClassLoader classLoader = FilteringMetricRegistryTest.class.getClassLoader();
+ Path yamlPath = writeResourceToPath(classLoader, confPath, "config/sidecar_metrics.yaml");
+ Injector injector = Guice.createInjector(new MainModule(yamlPath));
+ Server server = injector.getInstance(Server.class);
+ Vertx vertx = injector.getInstance(Vertx.class);
+
+ Checkpoint serverStarted = context.checkpoint();
+ Checkpoint waitUntilCheck = context.checkpoint();
+
+ vertx.eventBus().localConsumer(SidecarServerEvents.ON_SERVER_START.address(), message -> serverStarted.flag());
+
+ server.start()
+ .onFailure(context::failNow)
+ .onSuccess(v -> {
+ MetricRegistryFactory registryFactory = injector.getInstance(MetricRegistryFactory.class);
+ Pattern excludedPattern = Pattern.compile("vertx.eventbus.*");
+ FilteringMetricRegistry globalRegistry = (FilteringMetricRegistry) registryFactory.getOrCreate();
+ assertThat(globalRegistry.getIncludedMetrics().size()).isGreaterThanOrEqualTo(1);
+ assertThat(globalRegistry.getIncludedMetrics()
+ .keySet()
+ .stream()
+ .noneMatch(key -> excludedPattern.matcher(key).matches()))
+ .isTrue();
Review Comment:
You can instead directly do `noneMatch` within the assertion.
```suggestion
assertThat(globalRegistry.getIncludedMetrics()
.keySet()
.stream())
.noneMatch(key -> excludedPattern.matcher(key).matches()));
```
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560132544
##########
src/main/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistry.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.codahale.metrics.Timer;
+import io.vertx.core.impl.ConcurrentHashSet;
+
+/**
+ * Allows filtering of metrics based on configured allow list. Metrics are filtered out before registering them.
+ */
+public class FilteringMetricRegistry extends MetricRegistry
+{
+ private static final NoopMetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry(); // supplies no-op metrics
+ private final Predicate<String> isAllowed;
Review Comment:
For readability added `Predicate` extension
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560046404
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
+ try
+ {
+ metricRegistry = new FilteringMetricRegistry(this::isAllowed);
+ }
+ finally
+ {
+ readWriteLock.readLock().unlock();
+ }
+ SharedMetricRegistries.add(name, metricRegistry);
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ /**
+ * Check if the metric is allowed to register
+ * The evaluation order is inclusions first, then exclusions. In other words,
+ * a metric name is allowed if it is in the inclusions, but not in the exclusions.
+ * <p>
+ * Note that an empty inclusions means including all
+ *
+ * @param name metric name
+ * @return true if allowed; false otherwise
+ */
+ private boolean isAllowed(String name)
+ {
+ boolean included = inclusions.isEmpty() || inclusions.stream().anyMatch(filter -> filter.matches(name));
+ boolean excluded = exclusions.stream().anyMatch(filter -> filter.matches(name));
Review Comment:
In an example where configuration is
` include:
- type: "regex"
value: "vertx.*"
exclude:
- type: "regex"
value: "vertx.eventbus.*"`
We want to include all vert.x metrics except the event bus metrics. In that case, both included and excluded will be true. So it not necessarily a configuration error.
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560041121
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/MetricsFilteringConfigurationImpl.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.config.yaml;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Holds configuration needed for filtering out metrics
+ */
+public class MetricsFilteringConfigurationImpl implements MetricsFilteringConfiguration
+{
+ public static final String DEFAULT_TYPE = "regex";
+ public static final String DEFAULT_VALUE = ".*"; // include all metrics
+ @JsonProperty("type")
+ private final String type;
+ @JsonProperty("value")
+ private final String value;
+
+ public MetricsFilteringConfigurationImpl()
+ {
+ this(DEFAULT_TYPE, DEFAULT_VALUE);
+ }
+
+ public MetricsFilteringConfigurationImpl(String type, String value)
+ {
+ this.type = type;
+ this.value = value;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String type()
+ {
+ return type;
Review Comment:
You want to fail? We validate while parsing and then only create. If they dont match expected, we function as if that filter was not valid
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560053405
##########
src/main/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistry.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.codahale.metrics.Timer;
+import io.vertx.core.impl.ConcurrentHashSet;
+
+/**
+ * Allows filtering of metrics based on configured allow list. Metrics are filtered out before registering them.
+ */
+public class FilteringMetricRegistry extends MetricRegistry
+{
+ private static final NoopMetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry(); // supplies no-op metrics
+ private final Predicate<String> isAllowed;
Review Comment:
I feel adding `Predicate` to name is redundant. For consistency, lets keep it this way? since we don't add that for other members
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh merged PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560053215
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/MetricsFilteringConfigurationImpl.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.config.yaml;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Holds configuration needed for filtering out metrics
+ */
+public class MetricsFilteringConfigurationImpl implements MetricsFilteringConfiguration
+{
+ public static final String DEFAULT_TYPE = "regex";
+ public static final String DEFAULT_VALUE = ".*"; // include all metrics
+ @JsonProperty("type")
+ private final String type;
+ @JsonProperty("value")
+ private final String value;
+
+ public MetricsFilteringConfigurationImpl()
+ {
+ this(DEFAULT_TYPE, DEFAULT_VALUE);
+ }
+
+ public MetricsFilteringConfigurationImpl(String type, String value)
+ {
+ this.type = type;
+ this.value = value;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String type()
+ {
+ return type;
Review Comment:
I think it is a reasonable expectation to fail the service startup if misconfigured. As opposed to ignoring the error and have an operator spend a lot of time trying to figure out why something they think should be working is not really working. Take a look at this for example: [testInvalidClientAuth](https://github.com/apache/cassandra-sidecar/blob/trunk/src/test/java/org/apache/cassandra/sidecar/config/SidecarConfigurationTest.java#L176-L184)
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1559923918
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
Review Comment:
this lock doesn't make sense. I recommend synchronized or a simple lock instead
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/MetricsFilteringConfigurationImpl.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.config.yaml;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Holds configuration needed for filtering out metrics
+ */
+public class MetricsFilteringConfigurationImpl implements MetricsFilteringConfiguration
+{
+ public static final String DEFAULT_TYPE = "regex";
+ public static final String DEFAULT_VALUE = ".*"; // include all metrics
+ @JsonProperty("type")
+ private final String type;
+ @JsonProperty("value")
+ private final String value;
+
+ public MetricsFilteringConfigurationImpl()
+ {
+ this(DEFAULT_TYPE, DEFAULT_VALUE);
+ }
+
+ public MetricsFilteringConfigurationImpl(String type, String value)
+ {
+ this.type = type;
+ this.value = value;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String type()
+ {
+ return type;
Review Comment:
type is not validated. We should validate the allowed strings.
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
+ try
+ {
+ metricRegistry = new FilteringMetricRegistry(this::isAllowed);
+ }
+ finally
+ {
+ readWriteLock.readLock().unlock();
+ }
+ SharedMetricRegistries.add(name, metricRegistry);
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ /**
+ * Check if the metric is allowed to register
+ * The evaluation order is inclusions first, then exclusions. In other words,
+ * a metric name is allowed if it is in the inclusions, but not in the exclusions.
+ * <p>
+ * Note that an empty inclusions means including all
+ *
+ * @param name metric name
+ * @return true if allowed; false otherwise
+ */
+ private boolean isAllowed(String name)
+ {
+ boolean included = inclusions.isEmpty() || inclusions.stream().anyMatch(filter -> filter.matches(name));
+ boolean excluded = exclusions.stream().anyMatch(filter -> filter.matches(name));
Review Comment:
`if (included && excluded)` should we throw a configuration error?
##########
src/main/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistry.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.codahale.metrics.Timer;
+import io.vertx.core.impl.ConcurrentHashSet;
+
+/**
+ * Allows filtering of metrics based on configured allow list. Metrics are filtered out before registering them.
+ */
+public class FilteringMetricRegistry extends MetricRegistry
+{
+ private static final NoopMetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry(); // supplies no-op metrics
+ private final Predicate<String> isAllowed;
Review Comment:
Maybe call it predicate?
```suggestion
private final Predicate<String> isAllowedPredicate;
```
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
Review Comment:
use package private for testing?
```suggestion
MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
```
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560053405
##########
src/main/java/org/apache/cassandra/sidecar/metrics/FilteringMetricRegistry.java:
##########
@@ -0,0 +1,226 @@
+/*
+ * 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.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+import com.codahale.metrics.Counter;
+import com.codahale.metrics.Gauge;
+import com.codahale.metrics.Histogram;
+import com.codahale.metrics.Meter;
+import com.codahale.metrics.Metric;
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.NoopMetricRegistry;
+import com.codahale.metrics.Timer;
+import io.vertx.core.impl.ConcurrentHashSet;
+
+/**
+ * Allows filtering of metrics based on configured allow list. Metrics are filtered out before registering them.
+ */
+public class FilteringMetricRegistry extends MetricRegistry
+{
+ private static final NoopMetricRegistry NO_OP_METRIC_REGISTRY = new NoopMetricRegistry(); // supplies no-op metrics
+ private final Predicate<String> isAllowed;
Review Comment:
I feel adding `Predicate` to name is redundant. We don't add that for other members
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "sarankk (via GitHub)" <gi...@apache.org>.
sarankk commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560064139
##########
src/main/java/org/apache/cassandra/sidecar/config/yaml/MetricsFilteringConfigurationImpl.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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.config.yaml;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.cassandra.sidecar.config.MetricsFilteringConfiguration;
+
+/**
+ * Holds configuration needed for filtering out metrics
+ */
+public class MetricsFilteringConfigurationImpl implements MetricsFilteringConfiguration
+{
+ public static final String DEFAULT_TYPE = "regex";
+ public static final String DEFAULT_VALUE = ".*"; // include all metrics
+ @JsonProperty("type")
+ private final String type;
+ @JsonProperty("value")
+ private final String value;
+
+ public MetricsFilteringConfigurationImpl()
+ {
+ this(DEFAULT_TYPE, DEFAULT_VALUE);
+ }
+
+ public MetricsFilteringConfigurationImpl(String type, String value)
+ {
+ this.type = type;
+ this.value = value;
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String type()
+ {
+ return type;
Review Comment:
Got it will add the check to configImpl
--
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-115 Add FilteringMetricRegistry to allow filtering of metrics [cassandra-sidecar]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #109:
URL: https://github.com/apache/cassandra-sidecar/pull/109#discussion_r1560141747
##########
src/main/java/org/apache/cassandra/sidecar/metrics/MetricRegistryFactory.java:
##########
@@ -0,0 +1,116 @@
+/*
+ * 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.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import com.google.common.annotations.VisibleForTesting;
+
+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.config.SidecarConfiguration;
+
+/**
+ * Provider for getting {@link FilteringMetricRegistry} based on provided metrics configuration
+ */
+@Singleton
+public class MetricRegistryFactory
+{
+ private final String globalRegistryName;
+ private final ReadWriteLock readWriteLock;
+ private final List<MetricFilter> inclusions;
+ private final List<MetricFilter> exclusions;
+
+ @Inject
+ public MetricRegistryFactory(SidecarConfiguration sidecarConfiguration)
+ {
+ this(sidecarConfiguration.metricsConfiguration().registryName(),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().includeConfigurations()),
+ MetricFilter.parse(sidecarConfiguration.metricsConfiguration().excludeConfigurations()));
+ }
+
+ @VisibleForTesting
+ public MetricRegistryFactory(String globalRegistryName, List<MetricFilter> inclusions, List<MetricFilter> exclusions)
+ {
+ this.globalRegistryName = globalRegistryName;
+ this.readWriteLock = new ReentrantReadWriteLock();
+ this.inclusions = new ArrayList<>(inclusions);
+ this.exclusions = new ArrayList<>(exclusions);
+ }
+
+ public MetricRegistry getOrCreate()
+ {
+ return getOrCreate(globalRegistryName);
+ }
+
+ public MetricRegistry getOrCreate(int cassInstanceId)
+ {
+ String instanceRegistryName = globalRegistryName + "_" + cassInstanceId;
+ return getOrCreate(instanceRegistryName);
+ }
+
+ /**
+ * Provides a {@link FilteringMetricRegistry} with given name and provided filters in configuration.
+ * @param name registry name
+ * @return a {@link MetricRegistry} that can filter out metrics
+ */
+ public MetricRegistry getOrCreate(String name)
+ {
+ // the metric registry already exists
+ if (SharedMetricRegistries.names().contains(name))
+ {
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ FilteringMetricRegistry metricRegistry;
+ readWriteLock.readLock().lock();
+ try
+ {
+ metricRegistry = new FilteringMetricRegistry(this::isAllowed);
+ }
+ finally
+ {
+ readWriteLock.readLock().unlock();
+ }
+ SharedMetricRegistries.add(name, metricRegistry);
+ return SharedMetricRegistries.getOrCreate(name);
+ }
+
+ /**
+ * Check if the metric is allowed to register
+ * The evaluation order is inclusions first, then exclusions. In other words,
+ * a metric name is allowed if it is in the inclusions, but not in the exclusions.
+ * <p>
+ * Note that an empty inclusions means including all
+ *
+ * @param name metric name
+ * @return true if allowed; false otherwise
+ */
+ private boolean isAllowed(String name)
+ {
+ boolean included = inclusions.isEmpty() || inclusions.stream().anyMatch(filter -> filter.matches(name));
+ boolean excluded = exclusions.stream().anyMatch(filter -> filter.matches(name));
Review Comment:
thanks for the explanation, yeah this makes sense
--
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