apache / apache/druid

Migrate GroupByStatsMonitor.PerQueryStats to GroupByQueryMetrics

Open
#18,781 0 comments 0 reactions 0 assignees View on GitHub
Design Review Proposal
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.