yashmayya opened a new pull request, #18550: URL: https://github.com/apache/pinot/pull/18550
## Summary Introduces a dedicated `MseMetrics` abstraction for multi-stage engine (MSE) metrics, replacing direct `ServerMetrics` calls from the MSE engine call sites. The shim supports three emission modes selected by cluster config: - **`SERVER`** (default): forward to `ServerMetrics` so the existing `pinot.server.mse*` / `pinot.server.multiStage*` series stay exactly as today — no behavior change for existing deployments. - **`MSE`**: emit to a new role-agnostic `pinot.mse.*` namespace. - **`DUAL`**: emit to both during dashboard migration. ## Motivation The multi-stage engine is logically separate from the segment-scanning/storage path that historically owned `ServerMetrics`, but its metrics are co-mingled in the `pinot.server.*` namespace today. That makes it awkward to: - Attribute load to MSE vs SSE on shared JVMs. - Build engine-specific dashboards without globbing on metric-name prefixes. - Evolve MSE metric naming and units independently of `ServerMetrics`. - Surface MSE-engine emissions from any JVM that does not register `ServerMetrics` — the existing call sites hard-wire `ServerMetrics.get()`, which is the noop singleton outside of server JVMs. This change establishes a per-engine namespace as the pattern going forward, gated behind a config so existing dashboards keep working unchanged until operators migrate. ## Design - New `MseMeter` / `MseTimer` enums in `pinot-common` covering the existing `ServerMeter.MSE_*` / `ServerMeter.MULTI_STAGE_*` and corresponding `ServerTimer` entries, each carrying a reference to its counterpart so `SERVER` and `DUAL` modes can forward. - New `MseMetrics extends AbstractMetrics` singleton with `register()` / `get()` mirroring `ServerMetrics`. Mode-aware `addMeteredGlobalValue` / `getMeteredValue` / `addTimedValue` route emissions to the right destination(s). - `DUAL` mode hands back a fan-out `PinotMeter` wrapper (cached per `MseMeter`) so callers that resolve once and mark repeatedly (e.g. `MetricsExecutor`) hit both registries on every increment. - New cluster-config key `pinot.metrics.mse.mode` (default `SERVER`). Picked up at startup in `BaseServerStarter` and `BaseBrokerStarter` via the existing `applyClusterConfig` → `MseMetrics.registerFromConfig(...)`, before any MSE runtime component is constructed so cached `PinotMeter` handles bind to the right mode. - All MSE engine call sites migrated to `MseMetrics.get()`: `QueryServer`, `QueryRunner`, `OpChainSchedulerService`, `MailboxSendOperator`, and the `MultiStageOperator.Type` callbacks (renamed `updateServerMetrics` → `updateMseMetrics`). SSE leaf-stage paths still use `ServerMetrics` directly. - Prometheus JMX `broker.yml` catch-all generalized to the `(\w+)` group pattern already used by `server.yml` / `minion.yml` / `pinot.yml`, so `pinot.mse.*` series exported from broker JVMs are scraped consistently with the other roles. ## Compatibility - Default mode `SERVER` preserves all existing `pinot.server.mse*` / `pinot.server.multiStage*` series byte-for-byte. Existing dashboards and alerts continue to work without operator action. - The pre-registration `MseMetrics.NOOP` is in `SERVER` mode and resolves through `ServerMetrics.get()`, so any code path that emits before explicit registration still hits the same `ServerMetrics` handle today's code would have hit — no regression in tests or call sites that never initialize `MseMetrics`. - Removing the legacy `ServerMeter.MSE_*` / `MULTI_STAGE_*` and `ServerTimer` MSE entries is a separate follow-up; the migration path is documented on `MseMetricsMode`. ## Test plan - [x] `MseMetricsTest` — 14 unit tests covering all three modes, NOOP-fallback semantics, the SERVER-mode-on-noop-ServerMetrics case (drops silently as documented), MSE-mode without registered ServerMetrics (still emits), cached `DualPinotMeter` identity for repeated lookups, and `registerFromConfig` config parsing including the invalid-value fallback. - [x] `mseMeterExportedFromBrokerJmx` / `mseTimerExportedFromBrokerJmx` (15 + 5) and `mseMeterExportedFromServerJmx` / `mseTimerExportedFromServerJmx` (15 + 5) in `BrokerPrometheusMetricsTest` and `ServerPrometheusMetricsTest`. Each starts a real JMX→Prometheus exporter against the role's actual yml config, emits the entry, and asserts it surfaces as `pinot_mse_<name>_<measurement>`. Covers both the broker.yml regex generalization and the server.yml catch-all path. - [x] Existing `QueryServerTest` continues to pass unchanged — SERVER-mode forwarding preserves the same `ServerMeter.MSE_QUERIES` increment behavior the test asserts on. - [x] Pre-commit checks (spotless / checkstyle / license) clean on `pinot-common`, `pinot-spi`, `pinot-query-runtime`, `pinot-server`, `pinot-broker`, `pinot-dropwizard`, `pinot-yammer`. -- 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]
