yashmayya commented on code in PR #18550: URL: https://github.com/apache/pinot/pull/18550#discussion_r3278639718
########## 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: I think the reasoning was that this is a global cluster level config, but I can move it elsewhere. Edit: it doesn't really fit cleanly elsewhere and `Helix` section already has various other common cluster level configurations. -- 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]
