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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
