abhishekrb19 commented on code in PR #19026: URL: https://github.com/apache/druid/pull/19026#discussion_r2898778160
########## processing/src/main/java/org/apache/druid/java/util/emitter/core/MetricAllowlistParser.java: ########## @@ -0,0 +1,29 @@ +/* + * 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.druid.java.util.emitter.core; + +import com.fasterxml.jackson.databind.JsonNode; + +import java.util.Set; + +public interface MetricAllowlistParser +{ + Set<String> parse(JsonNode metricConfig, String source); +} Review Comment: Please add javadocs for common interfaces. ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/MetricAllowlistLoader.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.java.util.emitter.core; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Strings; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.ISE; + +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.util.Set; + +public final class MetricAllowlistLoader +{ + + public static final String DEFAULT_METRIC_ALLOWLIST_PATH = "defaultLoggingMetrics.json"; + + public static Set<String> loadAllowlist( + ObjectMapper mapper, + String allowlistPath, + MetricAllowlistParser parser + ) + { + try (final InputStream is = openAllowlistFile(allowlistPath)) { + return parser.parse(mapper.readTree(is), allowlistPath); + } + catch (IOException e) { + throw new ISE(e, "Failed to parse metric allowlist file [%s]", allowlistPath); + } + } + + private static InputStream openAllowlistFile(String allowlistPath) + { + if (Strings.isNullOrEmpty(allowlistPath)) { + throw new IAE("Metric allowlist file path is empty"); + } + try { + return new FileInputStream(allowlistPath); + } + catch (FileNotFoundException e) { + final InputStream classpathInputStream = MetricAllowlistLoader.class.getClassLoader().getResourceAsStream(allowlistPath); + if (classpathInputStream != null) { + return classpathInputStream; + } + throw new IAE(e, "Metric allowlist file [%s] not found", allowlistPath); + } Review Comment: DruidExceptions here as well similar to https://github.com/apache/druid/pull/19030/: https://github.com/apache/druid/pull/19030/changes#diff-e6a24d753a6939a848cd7909791a6ceaf358cb5976e7b3ed084912204778cb63R137-R145 ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/MetricAllowlistLoader.java: ########## @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.druid.java.util.emitter.core; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.base.Strings; +import org.apache.druid.java.util.common.IAE; +import org.apache.druid.java.util.common.ISE; + +import java.io.FileInputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.util.Set; + +public final class MetricAllowlistLoader +{ + + public static final String DEFAULT_METRIC_ALLOWLIST_PATH = "defaultLoggingMetrics.json"; + + public static Set<String> loadAllowlist( + ObjectMapper mapper, + String allowlistPath, + MetricAllowlistParser parser + ) + { + try (final InputStream is = openAllowlistFile(allowlistPath)) { + return parser.parse(mapper.readTree(is), allowlistPath); + } + catch (IOException e) { + throw new ISE(e, "Failed to parse metric allowlist file [%s]", allowlistPath); Review Comment: Please use DruidException - style guide ref: https://github.com/apache/druid/blob/master/dev/style-conventions.md#message-formatting-for-logs-and-exceptions ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/GlobalEmitterConfig.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.druid.java.util.emitter.core; + +import com.fasterxml.jackson.annotation.JsonProperty; + +import java.util.Optional; + +public class GlobalEmitterConfig +{ + @JsonProperty + boolean filterMetrics; + Review Comment: Please add javadocs ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/GlobalEmitterConfig.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.druid.java.util.emitter.core; + +import com.fasterxml.jackson.annotation.JsonProperty; + +import java.util.Optional; + +public class GlobalEmitterConfig +{ + @JsonProperty + boolean filterMetrics; + + @JsonProperty + String metricAllowlistPath; + Review Comment: These properties can be private-scoped ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/MetricAllowlistParsers.java: ########## @@ -0,0 +1,40 @@ +/* + * 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.druid.java.util.emitter.core; + +import com.fasterxml.jackson.databind.JsonNode; +import com.google.common.collect.ImmutableSet; +import org.apache.druid.java.util.common.ISE; + +import java.util.Set; + +public final class MetricAllowlistParsers +{ + + public static Set<String> parseMetricNameObject(JsonNode metricConfig, String source) + { + if (!metricConfig.isObject()) { + throw new ISE("Metric allowlist file [%s] must be a JSON object of metric names", source); + } + final ImmutableSet.Builder<String> metricNames = ImmutableSet.builder(); Review Comment: Does this work with the different defaultMetrics.json formats that emitters support (they look close enough but have slight differences). Could you also update some of the existing emitters like prometheus-emitter to exercise these code paths? ########## extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/PrometheusEmitter.java: ########## @@ -145,6 +167,9 @@ public void emit(Event event) private void emitMetric(ServiceMetricEvent metricEvent) { String name = metricEvent.getMetric(); + if (shouldFilterOutMetric(name)) { + return; + } Review Comment: Hmm, instead of doing this for every single emitter, what do you think about adding an abstract class `MetricFilteringEmitter` that overrides knows about the global config and have all the appropriate emitters just extend the implementation and delegate the filtering to it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [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]
