Jackie-Jiang commented on code in PR #18550: URL: https://github.com/apache/pinot/pull/18550#discussion_r3278158062
########## pinot-common/src/main/java/org/apache/pinot/common/metrics/MseMetrics.java: ########## @@ -0,0 +1,262 @@ +/** + * 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.pinot.common.metrics; + +import com.google.common.annotations.VisibleForTesting; +import java.util.Collections; +import java.util.EnumMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.metrics.NoopPinotMetricsRegistry; +import org.apache.pinot.spi.metrics.PinotMeter; +import org.apache.pinot.spi.metrics.PinotMetricsRegistry; +import org.apache.pinot.spi.utils.CommonConstants.Helix; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** Review Comment: (minor) Switch to markdown style ########## docker/images/pinot/etc/jmx_prometheus_javaagent/configs/broker.yml: ########## @@ -34,7 +34,8 @@ rules: - pattern: "\"org\\.apache\\.pinot\\.common\\.metrics\"<type=\"BrokerMetrics\", name=\"pinot\\.broker\\.(uncaughtGet|uncaughtPost|queryRejected|requestCompilation|resourceMissing)Exceptions\"><>(\\w+)" name: "pinot_broker_exceptions_$1_$2" cache: true -# All global gauge/meters/timers -- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?, name=\"?pinot\\.broker\\.(\\w+)\"?><>(\\w+)" - name: "pinot_broker_$1_$2" +# All global gauge/meters/timers. Group-flexible at the prefix so non-broker MBean groups +# registered in this JVM (e.g. pinot.mse.* from the multi-stage engine emitter) are also exported. +- pattern: "\"?org\\.apache\\.pinot\\.common\\.metrics\"?<type=\"?\\w+\"?, name=\"?pinot\\.(\\w+)\\.(\\w+)\"?><>(\\w+)" Review Comment: Do you need to do the same for server? Say we want to migrate to mse metric that can be emitted on server ########## pinot-common/src/main/java/org/apache/pinot/common/metrics/MseGauge.java: ########## @@ -0,0 +1,55 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.common.metrics; + +import org.apache.pinot.common.Utils; + + +/** + * Gauges for {@link MseMetrics}. Empty for now — MSE metrics today are global meters and timers. + * Present to satisfy the {@link AbstractMetrics} generic parameter and to provide a place to grow. + */ +public enum MseGauge implements AbstractMetrics.Gauge { + PLACEHOLDER("count", true); Review Comment: (minor) Is this required? ########## pinot-common/src/main/java/org/apache/pinot/common/metrics/MseMetrics.java: ########## @@ -0,0 +1,262 @@ +/** + * 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.pinot.common.metrics; + +import com.google.common.annotations.VisibleForTesting; +import java.util.Collections; +import java.util.EnumMap; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.metrics.NoopPinotMetricsRegistry; +import org.apache.pinot.spi.metrics.PinotMeter; +import org.apache.pinot.spi.metrics.PinotMetricsRegistry; +import org.apache.pinot.spi.utils.CommonConstants.Helix; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + + +/** + * Mode-aware metrics shim for the multi-stage engine. + * + * <p>All MSE engine call sites route through {@link #get()} instead of {@link ServerMetrics} + * directly. The active {@link MseMetricsMode} controls where emissions land: + * <ul> + * <li>{@link MseMetricsMode#SERVER} (default): forwarded to {@link ServerMetrics} so existing + * {@code pinot.server.*} dashboards continue to work unchanged.</li> + * <li>{@link MseMetricsMode#MSE}: emitted to this instance's own {@code pinot.mse.*} registry.</li> + * <li>{@link MseMetricsMode#DUAL}: emitted to both, for dashboard migration windows.</li> + * </ul> + * + * <p>The pre-registration {@link #NOOP} is in {@code SERVER} mode, so call sites that emit before + * any explicit registration behave the same as if they had called {@link ServerMetrics} directly. + * Because SERVER mode resolves the underlying handle through {@link ServerMetrics#get()}, + * emissions land in the noop registry whenever {@link ServerMetrics#register} has not been called + * on the local JVM; in that case the {@code MSE} or {@code DUAL} mode must be selected for the + * series to surface. + * + * <p><b>Initialization order:</b> call {@link #registerFromConfig} before constructing components + * that resolve a {@link PinotMeter} handle once and cache it for the JVM lifetime (notably the MSE + * {@code MetricsExecutor} cached handles and the inner {@code Metrics} class in + * {@code OpChainSchedulerService}). The server and broker starters preserve this ordering by + * registering immediately after {@code ServerMetrics.register} / {@code BrokerMetrics.register} + * and before any MSE runtime component is built. + */ +public class MseMetrics extends AbstractMetrics<MseQueryPhase, MseMeter, MseGauge, MseTimer> { + + public static final String METRIC_PREFIX = "pinot.mse."; + + private static final Logger LOGGER = LoggerFactory.getLogger(MseMetrics.class); + + private static final MseMetrics NOOP = new MseMetrics(MseMetricsMode.SERVER, new NoopPinotMetricsRegistry()); + private static final AtomicReference<MseMetrics> INSTANCE = new AtomicReference<>(NOOP); + + /** + * Register {@code mseMetrics} as the JVM-wide instance. Returns {@code true} if installed; + * {@code false} if another instance was already registered (compare-and-set semantics, matching + * {@link ServerMetrics#register}). + */ + public static boolean register(MseMetrics mseMetrics) { + return INSTANCE.compareAndSet(NOOP, mseMetrics); + } + + @VisibleForTesting + public static void deregister() { + INSTANCE.set(NOOP); + } + + public static MseMetrics get() { + return INSTANCE.get(); + } + + /** + * Reads {@link Helix#CONFIG_OF_MSE_METRICS_MODE} from {@code instanceConfig} (which already + * contains cluster-config keys merged in via + * {@code ServiceStartableUtils.applyClusterConfig(...)} at startup) and registers a new + * {@link MseMetrics} instance with the resolved mode. {@link MseMetricsMode#SERVER} reuses + * {@link NoopPinotMetricsRegistry} since no {@code pinot.mse.*} series are emitted in that mode. + * Subsequent calls in the same JVM are no-ops (compare-and-set with the NOOP placeholder). + */ + public static void registerFromConfig(PinotConfiguration instanceConfig, PinotMetricsRegistry metricsRegistry) { + String modeStr = instanceConfig.getProperty(Helix.CONFIG_OF_MSE_METRICS_MODE, Helix.DEFAULT_MSE_METRICS_MODE); Review Comment: Why is this under `Helix`? ########## pinot-common/src/main/java/org/apache/pinot/common/metrics/MseMeter.java: ########## @@ -0,0 +1,81 @@ +/** + * 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.pinot.common.metrics; + +import java.util.Objects; +import org.apache.pinot.common.Utils; + + +/** + * Meters for the multi-stage engine, emitted via {@link MseMetrics} as {@code pinot.mse.*} when + * the cluster is configured for {@link MseMetricsMode#MSE} or {@link MseMetricsMode#DUAL}. + * + * <p>Each entry carries the corresponding {@link ServerMeter} so {@link MseMetricsMode#SERVER} and + * {@link MseMetricsMode#DUAL} can forward to the existing {@code pinot.server.*} series. + */ +public enum MseMeter implements AbstractMetrics.Meter { + QUERIES("queries", true, ServerMeter.MSE_QUERIES), + OPCHAINS_STARTED("opchains", true, ServerMeter.MSE_OPCHAINS_STARTED), + OPCHAINS_COMPLETED("opchains", true, ServerMeter.MSE_OPCHAINS_COMPLETED), + CPU_EXECUTION_TIME_MS("milliseconds", true, ServerMeter.MSE_CPU_EXECUTION_TIME_MS), + MEMORY_ALLOCATED_BYTES("bytes", true, ServerMeter.MSE_MEMORY_ALLOCATED_BYTES), + EMITTED_ROWS("rows", true, ServerMeter.MSE_EMITTED_ROWS), + RUNNER_STARTED_TASKS("tasks", true, ServerMeter.MULTI_STAGE_RUNNER_STARTED_TASKS), + RUNNER_COMPLETED_TASKS("tasks", true, ServerMeter.MULTI_STAGE_RUNNER_COMPLETED_TASKS), + SUBMISSION_STARTED_TASKS("tasks", true, ServerMeter.MULTI_STAGE_SUBMISSION_STARTED_TASKS), + SUBMISSION_COMPLETED_TASKS("tasks", true, ServerMeter.MULTI_STAGE_SUBMISSION_COMPLETED_TASKS), + HASH_JOIN_TIMES_MAX_ROWS_REACHED("times", true, ServerMeter.HASH_JOIN_TIMES_MAX_ROWS_REACHED), + WINDOW_TIMES_MAX_ROWS_REACHED("times", true, ServerMeter.WINDOW_TIMES_MAX_ROWS_REACHED), + IN_MEMORY_MESSAGES("messages", true, ServerMeter.MULTI_STAGE_IN_MEMORY_MESSAGES), + RAW_MESSAGES("messages", true, ServerMeter.MULTI_STAGE_RAW_MESSAGES), + RAW_BYTES("bytes", true, ServerMeter.MULTI_STAGE_RAW_BYTES); + + private final String _meterName; + private final String _unit; + private final boolean _global; + private final ServerMeter _serverMeter; + + MseMeter(String unit, boolean global, ServerMeter serverMeter) { Review Comment: Is the fallback mandatory? Can we add new metrics without fallback? -- 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]
