You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "yashmayya (via GitHub)" <gi...@apache.org> on 2026/05/18 22:11:53 UTC
[PR] [multi-stage] Introduce MseMetricsEmitter for MSE engine metric emission [pinot]
yashmayya opened a new pull request, #18526:
URL: https://github.com/apache/pinot/pull/18526
## Background
The Multi-Stage Engine intermediate-stage code in `pinot-query-runtime` emits engine metrics by directly calling `ServerMetrics.get()`. `ServerMetrics` is registered only in `BaseServerStarter`. Two concrete bugs follow from this coupling:
1. **`OpChainSchedulerService.Metrics` NOOP-binding** (`pinot-query-runtime/.../executor/OpChainSchedulerService.java`). The inner class field-initializes five `PinotMeter` handles via `ServerMeter.X.getGlobalMeter()`, which resolves against whichever `ServerMetrics` is registered at that moment. If construction happens before `ServerMetrics.register(...)` runs — which happens in unit tests, embedded query-runtime harnesses, and any non-server context that constructs the scheduler directly — the captured handles are permanently bound to the NOOP registry. The op-chain started/completed counters and the per-op-chain CPU/memory/row counters then silently emit zero for the lifetime of the JVM. No later \`ServerMetrics.register()\` call can fix the captured references.
2. **`MultiStageBrokerRequestHandler` NOOP-binding** (`pinot-broker/.../requesthandler/MultiStageBrokerRequestHandler.java`). Same field-init-time anti-pattern for four `BrokerMeter.MSE_STAGES_*` and `BrokerMeter.MSE_OPCHAINS_*` handles, resolved before `BrokerMetrics.register(...)` is guaranteed to have run.
Beyond the bug fixes, the static-singleton coupling makes the MSE engine layer hard to test in isolation: every test that wants to assert metric emission must stand up a full `ServerMetrics` singleton and remember to deregister it. There is no way to install a recording fixture without touching global state.
## Change
This PR introduces `MseMetricsEmitter`, a thin pluggable interface in `pinot-common`, that MSE engine code calls instead of `ServerMetrics.get()` directly. The interface is keyed on the existing `ServerMeter` / `ServerTimer` enum constants — no new metric names, no new enums, no new Prometheus namespaces.
- `MseMetricsEmitter` exposes three methods: `addMeteredGlobalValue(ServerMeter, long)`, `getMeteredValue(ServerMeter)` (for `MetricsExecutor` handles), `addTimedValue(ServerTimer, long, TimeUnit)`.
- `ServerMetricsEmitter` is the default implementation. It forwards every call to `ServerMetrics.get()` at *call time* (not at construction). This eliminates the NOOP-binding hazard.
- `BaseServerStarter` registers `ServerMetricsEmitter` immediately after `ServerMetrics.register(_serverMetrics)`. A second `register(...)` call (e.g. an integration test that restarts the server in the same JVM) silently no-ops with an INFO log, matching the `ServerMetrics.register` pattern.
- MSE emission sites — `QueryServer.submit()`, `QueryServer` constructor, `QueryRunner.init()`, `OpChainSchedulerService.Metrics`, `MultiStageOperator.Type.updateServerMetrics`, `MailboxSendOperator.updateMetrics` — switch from `ServerMetrics.get()` to `MseMetricsEmitter.get()`.
- `OpChainSchedulerService.Metrics` is rewritten to drop the field-cached `PinotMeter` handles entirely; each emission resolves the emitter at call time. Op-chain lifecycle events are coarse (once per stage worker, not per row), so the additional indirection is unmeasurable.
- `MultiStageBrokerRequestHandler` is fixed independently of the emitter — the four field-cached handles are replaced with `volatile` fields populated lazily via `BrokerMetrics.get().addMeteredGlobalValue(meter, count, reusedMeter)`. These are broker dispatch counters that stay on `BrokerMetrics`; they do not go through `MseMetricsEmitter`.
## What does NOT change
- No metric names change. \`pinot.server.mse*\` Prometheus output is identical.
- No JMX exporter rules change.
- No new enum entries on `ServerMeter`, `BrokerMeter`, `ServerTimer`, `BrokerTimer`, `ServerGauge`, `BrokerGauge`.
- No new Prometheus namespace.
- No new feature flag.
- `GrpcMailboxServer` is untouched — its existing `InstanceType.BROKER`/`SERVER` branch correctly routes the mailbox memory gauges through `BrokerMetrics` / `ServerMetrics` respectively.
## Compatibility
- Source and binary compatible — the only public class signatures touched are on `MultiStageOperator.Type` (a public enum nested inside a `pinot-query-runtime` class; the `updateServerMetrics` method signature changes from `(StatMap, ServerMetrics)` to `(StatMap, MseMetricsEmitter)`). This is an internal execution-layer API; no external caller in the OSS tree exercises it.
- Rolling-upgrade safe — registration happens before any MSE code runs; no wire-protocol or schema changes.
## Tests
- `MseMetricsEmitterTest` — singleton lifecycle (NOOP fallback, first-write-wins, deregister).
- `ServerMetricsEmitterTest` — default impl correctly forwards meter, timer, and `MetricsExecutor` handle lookups to `ServerMetrics`. Includes \`testLateServerMetricsRegistrationStillEmitsCorrectly\` which proves the NOOP-binding hazard is gone (construct emitter → emit → register fresh `ServerMetrics` → re-emit → assert the new registry sees the increment).
- `OpChainSchedulerServiceMetricsTest` — regression test for the field-cached NOOP-binding bug. Constructs the scheduler before any metrics registration; registers; drives an op-chain to completion; asserts the live registry sees `MSE_OPCHAINS_STARTED`.
- `QueryServerTest` — existing tests pass unchanged, exercising the full `QueryServer.submit()` → `MseMetricsEmitter` → `ServerMetrics` path. Test setup registers `ServerMetricsEmitter` alongside the existing `ServerMetrics` setup.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [multi-stage] Introduce MseMetricsEmitter for MSE engine metric emission [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18526:
URL: https://github.com/apache/pinot/pull/18526#issuecomment-4483178821
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18526?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.75862%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.79%. Comparing base ([`aec5428`](https://app.codecov.io/gh/apache/pinot/commit/aec54289c51d9188cd2e5d5fe3d34550cf20503d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`50712d4`](https://app.codecov.io/gh/apache/pinot/commit/50712d46f441903d70f98da9dcf5c0e0808d7a48?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 25 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/18526?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../pinot/common/metrics/MseMetricsEmitterHolder.java](https://app.codecov.io/gh/apache/pinot/pull/18526?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FMseMetricsEmitterHolder.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Nc2VNZXRyaWNzRW1pdHRlckhvbGRlci5qYXZh) | 52.94% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...not/query/runtime/operator/MultiStageOperator.java](https://app.codecov.io/gh/apache/pinot/pull/18526?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Foperator%2FMultiStageOperator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NdWx0aVN0YWdlT3BlcmF0b3IuamF2YQ==) | 83.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/18526?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (aec5428) and HEAD (50712d4). Click for more details.
>
> <details><summary>HEAD has 4 uploads less than BASE</summary>
>
>| Flag | BASE (aec5428) | HEAD (50712d4) |
>|------|------|------|
>|java-21|2|1|
>|unittests|2|1|
>|temurin|2|1|
>|unittests2|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18526 +/- ##
============================================
- Coverage 63.72% 55.79% -7.94%
+ Complexity 1679 815 -864
============================================
Files 3265 2564 -701
Lines 199745 148557 -51188
Branches 31012 24013 -6999
============================================
- Hits 127296 82881 -44415
+ Misses 62296 58573 -3723
+ Partials 10153 7103 -3050
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <82.75%> (-7.94%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <82.75%> (-7.94%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <82.75%> (-7.94%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.79% <82.75%> (+0.01%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18526/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18526?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] [multi-stage] Introduce MseMetricsEmitter for MSE engine metric emission [pinot]
Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya closed pull request #18526: [multi-stage] Introduce MseMetricsEmitter for MSE engine metric emission
URL: https://github.com/apache/pinot/pull/18526
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org