Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit QueryCountMetrics for SQL Queries #17452

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jtuglu-netflix
Copy link
Contributor

@jtuglu-netflix jtuglu-netflix commented Nov 6, 2024

Fixes #11652.

Description

This pulls out the query count metrics logic from QueryResource and places it into BaseQueryCounterResource, which allows for SqlResource to share this codepath. An alternative approach is here, which avoids shared logic between the two resources. As stated in the issue, the ideal change would be to have SqlResource inherit, or simply pass on, all query handling to QueryResource, but that seemed a bit out of scope for this PR (but I can do this).

Notes:

  • I removed the get/increment handles from both QueryResource and SqlResource for simplicity, and simply made the counter package-private for accessing in tests. I can add the get() accessors back if necessary.

Release note


Key changed/added classes in this PR
  • extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/controller/http/DartSqlResource.java
  • extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/dart/controller/http/DartSqlResourceTest.java
  • quidem-ut/src/main/java/org/apache/druid/quidem/ExposedAsBrokerQueryComponentSupplierWrapper.java
  • server/src/main/java/org/apache/druid/server/BaseQueryCountResource.java
  • server/src/main/java/org/apache/druid/server/BrokerQueryResource.java
  • server/src/main/java/org/apache/druid/server/QueryMetricCounter.java
  • server/src/main/java/org/apache/druid/server/QueryResource.java
  • server/src/main/java/org/apache/druid/server/QueryResultPusher.java
  • server/src/test/java/org/apache/druid/server/QueryResourceTest.java
  • server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java
  • services/src/main/java/org/apache/druid/cli/CliBroker.java
  • services/src/main/java/org/apache/druid/cli/CliHistorical.java
  • services/src/main/java/org/apache/druid/guice/QueryablePeonModule.java
  • sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
  • sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@github-actions github-actions bot added Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Nov 6, 2024
@jtuglu-netflix jtuglu-netflix changed the title Create base counter query count class sharable by query resource types Emit QueryCountMetrics for SQL Queries Nov 6, 2024
@jtuglu-netflix jtuglu-netflix force-pushed the add-query-metrics-for-sql-base-query-class-member branch from 56d95c5 to bd5679f Compare November 6, 2024 00:55
This also fixes a bug in the QueryResultPusher code regarding handling of exceptions.
@jtuglu-netflix jtuglu-netflix force-pushed the add-query-metrics-for-sql-base-query-class-member branch from 219de13 to 03d0fd3 Compare November 6, 2024 07:16
@jtuglu-netflix jtuglu-netflix marked this pull request as ready for review November 8, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QueryCountStatsMonitor doesn't track sql query metrics
1 participant