Migrate GroupByStatsMonitor.PerQueryStats to GroupByQueryMetrics
- Dominant language
- Java
- Stars
- 14.1k
- Forks
- 3.8k
- Avg merge
- 2d 58m
- Merged PRs (30d)
- 233
Description
Possibly fixes #17944
### Motivation
Currently, we have GroupByStatsMonitor, which provide metrics for GroupBy queries by:
Aggregating results of all GroupBy metrics in a certain time interval.
Emit them regularly through ServiceEmitter.
There are still some limitations of the GroupByStatsMonitor:
1. Which query (SQL / Native) is emitting the stats?
2. Which datasource is being queried?
These limitations lead to more and more metrics being introduced, which does not really answer the questions asked above.
#### Previous Attempts
- Adding merge buffers used per query: #17945
- Adding `MAX` metrics to retrieve more details: [#18731](https://github.com/apache/druid/pull/18731#discussion_r2543534386)
After migrating to GroupByQueryMetrics, we can use the dimensions (e.g. ID, DATASOURCE, and more) to help us answer the above question.
### Proposed changes
- Replace the PerQueryStats accumulator and GroupByStatsProvider’s resource‑ID map with enhanced GroupByQueryMetrics instances. These metrics now track merge‑buffer acquisition time, spill bytes, dictionary size, and can be reported per query using existing dimensions (id, datasource, sqlQueryId, etc.).
- Collate the GroupByQueryMetrics in the QueryRunner level (Maybe wrap `GroupByMergingQueryRunner` in a `MetricsEmittingQueryRunner`?)
- GroupByStatsProvider no longer exposes PerQueryStats. Internal extensions/tests that constructed those objects need to switch to `GroupByQueryMetrics` or `QueryPlus` objects, but this does not affect public interfaces.
### Rationale
- Using `GroupByQueryMetrics` leverages an existing, dimension‑rich metrics pipeline instead of growing the ad‑hoc `GroupByStatsMonitor`.
- Providing these metrics by default, operators will not need to dive into documentation to retrieve the qualified class name of `GroupByStatsMonitor` to append to `druid.monitoring.monitors`.
- Encourage future metrics to be added to `GroupByQueryMetrics` in favour of `GroupByStatsMonitor`, following after the style of the entire repository.
### Operational impact
- No user‑visible API changes: SQL/native queries, JSON specs, and metric names remain the same. Existing dashboards based on GroupByStatsMonitor continue working.
- Cluster operators gain richer per‑query telemetry automatically; no configuration changes are required beyond upgrading.
### Test plan
- Unit tests ensuring GroupByStatsProvider aggregates metrics correctly from GroupByQueryMetrics.
- Integration tests for query metrics.
### Future work
- Evaluate whether GroupByStatsMonitor is still needed once operators rely on `GroupByQueryMetrics`. We may need to find a new home for metrics that show the number of used / pending merge buffers in the pool.
Contributor guide
Research direction
Start by tracing GroupByStatsProvider, its PerQueryStats and resource-ID map, then inspect GroupByQueryMetrics and the GroupByMergingQueryRunner or possible MetricsEmittingQueryRunner integration. Review the planned unit tests for aggregation and integration tests for query metrics. Done means metrics are collated per query with existing dimensions, internal tests use the new objects, and public interfaces and existing metric names remain unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, observability
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100