FrankChen021 commented on code in PR #19026: URL: https://github.com/apache/druid/pull/19026#discussion_r3934225466
########## processing/src/main/java/org/apache/druid/java/util/emitter/core/MetricAllowlistLoader.java: ########## @@ -0,0 +1,104 @@ +/* + * 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.error.DruidException; + +import java.io.File; +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 Set<String> loadAllowlistFromFile( + final ObjectMapper mapper, + final String allowlistPath, + final MetricAllowlistParser parser + ) + { + validateAllowlistPath(allowlistPath); + + final File allowlistFile = new File(allowlistPath); + + try (final InputStream is = new FileInputStream(allowlistFile)) { + return parseAllowlist(mapper, is, allowlistFile.getPath(), parser); + } + catch (FileNotFoundException e) { + throw DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.NOT_FOUND) + .build("Metric spec file path [%s] was not found.", allowlistFile.getPath()); + } + catch (IOException e) { + throw DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.RUNTIME_FAILURE) + .build("Failed to parse metric spec file path [%s].", allowlistFile.getPath()); + } + } + + public static Set<String> loadAllowlistFromClasspath( + final ObjectMapper mapper, + final String resourcePath, + final MetricAllowlistParser parser + ) + { + validateAllowlistPath(resourcePath); + + final InputStream classpathInputStream = MetricAllowlistLoader.class.getClassLoader().getResourceAsStream(resourcePath); Review Comment: [P1] Load extension resources with the extension classloader `MetricAllowlistLoader` is part of the core `processing` jar, so `MetricAllowlistLoader.class.getClassLoader()` is the Druid/core classloader. The default Prometheus spec, however, is packaged only in the separately loaded `prometheus-emitter` extension jar. With the normal external-extension classloader setup, enabling filtering without a custom path makes this lookup return null and throws `NOT_FOUND` during `PrometheusEmitter` construction, so the Prometheus emitter cannot start. Please load this resource through the Prometheus/context classloader or pass the resource-owning classloader into the shared loader. ########## extensions-contrib/prometheus-emitter/src/main/java/org/apache/druid/emitter/prometheus/Metrics.java: ########## @@ -66,7 +66,7 @@ public DimensionsAndCollector getByName(String name, String service) public Metrics(PrometheusEmitterConfig config) { String namespace = config.getNamespace(); - String path = config.getDimensionMapPath(); + String path = config.getMetricSpecPath().orElse(config.getDimensionMapPath()); Review Comment: [P1] Keep the filter allowlist separate from the Prometheus mapping `metricSpecPath` is the shared filter-only allowlist path (the documented shape permits values such as `[]`), while `dimensionMapPath` contains Prometheus `Metric` definitions. Preferring `metricSpecPath` here unconditionally makes a custom allowlist override a valid dimension map; `readConfig` then tries to deserialize entries like `"query/time": []` as `Metric` objects and the emitter construction fails (or registers no collectors for object values). This also changes behavior when filtering is false, even though the shared config says the path is only used for filtering. Keep `Metrics` on `dimensionMapPath`/its default and use `metricSpecPath` only for the allowlist. ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/Emitters.java: ########## @@ -145,9 +157,9 @@ static Map<String, Object> makeLoggingMap(Properties props) "shouldFilterMetrics", Boolean.parseBoolean(props.getProperty("org.apache.druid.java.util.emitter.logging.shouldFilterMetrics")) ); } - if (props.containsKey("org.apache.druid.java.util.emitter.logging.allowedMetricsPath")) { + if (props.containsKey("org.apache.druid.java.util.emitter.logging.metricSpecPath")) { Review Comment: [P2] Preserve the legacy logging path alias This replaces the existing `org.apache.druid.java.util.emitter.logging.allowedMetricsPath` property with the new name without translating the old key. A deployment using the previously supported `druid.emitter.logging.allowedMetricsPath` (or the legacy `org.apache...logging.allowedMetricsPath`) will still have filtering enabled but its custom file will be omitted, causing a silent fallback to the bundled allowlist; the old `LoggingEmitterConfig` getter is removed as well. Please accept the old property as a deprecated alias and map it to `metricSpecPath`. ########## processing/src/main/resources/defaultMetrics.json: ########## @@ -94,56 +111,34 @@ "metadata/kill/datasource/count": [], "metadata/kill/rule/count": [], "metadata/kill/supervisor/count": [], - "metadatacache/backfill/count": [], "metadatacache/init/time": [], - "metadatacache/refresh/count": [], - "metadatacache/refresh/time": [], - "metadatacache/schemaPoll/count": [], - "metadatacache/schemaPoll/failed": [], "metadatacache/schemaPoll/time": [], + "query/byteLimit/exceeded/count": [], "query/bytes": [], - "query/cache/delta/averageBytes": [], - "query/cache/delta/errors": [], - "query/cache/delta/evictions": [], - "query/cache/delta/hitRate": [], - "query/cache/delta/hits": [], - "query/cache/delta/misses": [], - "query/cache/delta/numEntries": [], - "query/cache/delta/put/error": [], - "query/cache/delta/put/ok": [], - "query/cache/delta/put/oversized": [], - "query/cache/delta/sizeBytes": [], - "query/cache/delta/timeouts": [], - "query/cache/total/averageBytes": [], - "query/cache/total/errors": [], - "query/cache/total/evictions": [], - "query/cache/total/hitRate": [], - "query/cache/total/hits": [], - "query/cache/total/misses": [], - "query/cache/total/numEntries": [], - "query/cache/total/put/error": [], - "query/cache/total/put/ok": [], - "query/cache/total/put/oversized": [], - "query/cache/total/sizeBytes": [], - "query/cache/total/timeouts": [], + "query/cache/memcached/delta": [], Review Comment: [P1] Preserve active metrics in the default allowlist When filtering is enabled without a custom spec, the new shared emitters use this file. This change removes the existing `query/cache/delta/*` and `query/cache/total/*` entries, all `kafka/consumer/*` entries, and `service/heartbeat`, even though `CacheMonitor`, `KafkaConsumerMonitor`, and `ServiceStatusMonitor` still emit those names (and they were present in the previous default). Those events will now be silently dropped by the logging, HTTP, and parametrized emitters for users relying on the default allowlist. Please retain the active previously allowed metrics or otherwise preserve this compatibility. ########## processing/src/main/java/org/apache/druid/java/util/emitter/core/GlobalEmitterConfig.java: ########## @@ -0,0 +1,70 @@ +/* + * 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; + +/** + * Shared metric-filtering configuration for emitter implementations. + */ +public class GlobalEmitterConfig +{ + /** + * When true, only metrics listed in the allowed metrics configuration are emitted. + * If {@link #metricSpecPath} is null/empty, the bundled default allowlist + * (`defaultMetrics.json` on the classpath) is used. If a path is provided, + * it is loaded from that file instead. + * Defaults to false (emit all metrics). + */ + @JsonProperty + private boolean shouldFilterMetrics; + + /** + * Optional path to a JSON file containing a JSON object keyed by allowed metric names, + * for example `{"query/time": [], "jvm/gc/cpu": []}`. + * Only used when {@link #shouldFilterMetrics} is true. + * If null or empty, the bundled default resource (`defaultMetrics.json`) is loaded + * from the classpath. + */ + @JsonProperty + private String metricSpecPath; + + public boolean isShouldFilterMetrics() + { + return shouldFilterMetrics; + } + + public Optional<String> getMetricSpecPath() + { + return Optional.ofNullable(metricSpecPath); Review Comment: [P2] Treat an empty metric path as absent The config Javadocs and the logging documentation promise that a null or empty `metricSpecPath` selects the bundled default. `Optional.ofNullable("")` is present, so `AbstractFilteringEmitter.loadAllowedMetricNames` sends the empty string to `loadAllowlistFromFile`, whose validation rejects it. Consequently, enabling filtering with `metricSpecPath: ""` fails construction for the HTTP, parametrized, and Prometheus emitters (only LoggingEmitter currently normalizes it). Normalize empty values in the shared getter/helper. -- 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]
