Skip to content

Conversation

@GWphua
Copy link
Contributor

@GWphua GWphua commented Nov 14, 2025

Description

Log query metrics to QueryStats HashMap

Allow QueryMetrics to participate in populating the map of QueryStats. This allows for polymorphic behaviour, in the case that we want to add per-query logging based on query type (e.g. GroupByQueryMetrics can now log additional metrics such as number of merge buffer used).

Key changed/added classes in this PR
  • DefaultQueryMetrics
  • DefaultSearchQueryMetrics
  • QueryMetrics
  • QueryLifecycle

This PR has:

  • been self-reviewed.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster. (Pending)

@GWphua GWphua closed this Nov 21, 2025
@aho135
Copy link
Contributor

aho135 commented Nov 24, 2025

Hi @GWphua I had some interest in this PR, mainly because I'm interested in adding query level metrics for GroupBy queries which capture mergeBufferAcquisitionTimeNs, spilledBytes and mergeDictionarySize. I'd like to have metrics for these at the query level with a sqlQueryId dimension.

Just curious if you were thinking on similar lines here, and why you closed this one. Thanks!

@GWphua
Copy link
Contributor Author

GWphua commented Nov 26, 2025

Hi @aho135. Yes this PR is an attempt to do exactly what you described. But there's a catch that I missed -- The QueryMetrics are cleared after emission.

One good example to look at is the Sequence object created in CPUTimeMetricQueryRunner:

if (cpuTimeNs > 0) {
  responseContext.addCpuNanos(cpuTimeNs);
  queryWithMetrics.getQueryMetrics().reportCpuTime(cpuTimeNs).emit(emitter);
}

In other words, when we finish the query, and reach emitLogsAndMetrics, the groupBy stats are probably cleared by other QueryRunners already...

We will need to either

  1. Report the GroupBy metrics directly in emitLogsAndMetrics, then emit after setting populating statsMap with metrics.
  2. Do not clear the metrics (Not sure about this, may cause metrics to double-report? Correct me if I'm wrong)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants