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.

Sign up for GitHub

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

Jump to bottom

SqlResource: Metrics, request logs, cancellation #4047

Closed
gianm opened this issue Mar 13, 2017 · 6 comments
Closed

SqlResource: Metrics, request logs, cancellation #4047

gianm opened this issue Mar 13, 2017 · 6 comments
Labels
Area - SQL Improvement stale

Comments

@gianm
Copy link
Contributor

gianm commented Mar 13, 2017 •
edited
Loading

SqlResource should be up to the standards of QueryResource:

  • Emit metrics for SQL queries. This could be query/sql/time to differentiate it from query/time.
  • Emit metrics like query/time for the Druid native queries that underlie a SQL query. There might be more than one of these per SQL query. Fixed in Add metrics to the native queries underpinning SQL. #4561
  • Emit request logs for the Druid native queries that underlie a SQL query. Fixed in Add metrics to the native queries underpinning SQL. #4561
  • Emit friendly errors for QueryInterruptedException and any other cases that QueryResource emits friendly errors. This can be tested by setting a really low timeout and confirming you get a friendly JSON error rather than unfriendly HTML. Fixed in SQL: Better error handling for HTTP API. #4053
  • Implement query cancellation, maybe by giving an "umbrella query ID" to all Druid native queries that underlie a SQL query, and returning that in the header of SqlResource's response, or maybe by assigning the same Druid query ID to all native queries (I think this would technically work although it would be a little strange).
  • Return query IDs on SQL queries so users can correlate it to metrics stores.

I think a lot of this could be accomplished by sharing code between QueryResource and SqlResource. Some of it (like a query/sql/time metric) would be unique to SqlResource.
@gianm gianm added this to the 0.10.1 milestone Mar 13, 2017
@gianm gianm changed the title SQL: Emit metrics, request logs, proper errors SqlResource: Metrics, request logs, proper errors Mar 13, 2017
@gianm gianm changed the title SqlResource: Metrics, request logs, proper errors SqlResource: Metrics, request logs, cancellation, proper errors Mar 13, 2017
@drcrallen
Copy link
Contributor

Why would metrics need to be different?

@gianm
Copy link
Contributor Author

gianm commented Mar 13, 2017

Because one SQL query can lead to multiple native Druid queries, there's an extra layer that QueryResource doesn't have to deal with.

@drcrallen
Copy link
Contributor

Would it make sense to have an extra dimension for those metrics instead?

@gianm
Copy link
Contributor Author

gianm commented Mar 14, 2017

I dunno, it depends on whether you think query/time means "the time it takes for an http call to return" or "the time it takes a native druid query to execute". Either one should be fine as long as the information is there.

With other things, we give different "layers" of a request different names, like query/time, query/node/time, query/segment/time, etc.

@gianm gianm mentioned this issue Mar 14, 2017
Merged
@gianm gianm changed the title SqlResource: Metrics, request logs, cancellation, proper errors SqlResource: Metrics, request logs, cancellation Mar 14, 2017
@gianm gianm modified the milestones: 0.10.2, 0.10.1 May 16, 2017
@leventov leventov modified the milestones: 0.11.0, 0.10.2 Jun 26, 2017
@gianm gianm mentioned this issue Jul 18, 2017
Merged
gianm added a commit to gianm/druid that referenced this issue Jul 18, 2017
@gianm
Add metrics to the native queries underpinning SQL.
66ccd29 
This is done by factoring out the metrics and request log emitting
code from QueryResource into a new QueryLifecycle class. That class
is used by both QueryResource and the SQL DruidSchema and QueryMaker.

Also fixes a couple of bugs in QueryResource:

- RequestLogLine start time was set to `TimeUnit.NANOSECONDS.toMillis(startNs)`,
  which is incorrect since absolute nanos cannot be converted to millis.
- DruidMetrics.makeRequestMetrics was called with null `query` on
  unparseable queries, which led to spurious "Unable to log query"
  errors.

Partial fix for apache#4047.
gianm added a commit to gianm/druid that referenced this issue Jul 24, 2017
@gianm
Add metrics to the native queries underpinning SQL.
65e5b51 
This is done by factoring out the metrics and request log emitting
code from QueryResource into a new QueryLifecycle class. That class
is used by both QueryResource and the SQL DruidSchema and QueryMaker.

Also fixes a couple of bugs in QueryResource:

- RequestLogLine start time was set to `TimeUnit.NANOSECONDS.toMillis(startNs)`,
  which is incorrect since absolute nanos cannot be converted to millis.
- DruidMetrics.makeRequestMetrics was called with null `query` on
  unparseable queries, which led to spurious "Unable to log query"
  errors.

Partial fix for apache#4047.
jon-wei pushed a commit that referenced this issue Jul 25, 2017
@gianm @jon-wei
Add metrics to the native queries underpinning SQL. (#4561)
5048ab3 
* Add metrics to the native queries underpinning SQL.

This is done by factoring out the metrics and request log emitting
code from QueryResource into a new QueryLifecycle class. That class
is used by both QueryResource and the SQL DruidSchema and QueryMaker.

Also fixes a couple of bugs in QueryResource:

- RequestLogLine start time was set to `TimeUnit.NANOSECONDS.toMillis(startNs)`,
  which is incorrect since absolute nanos cannot be converted to millis.
- DruidMetrics.makeRequestMetrics was called with null `query` on
  unparseable queries, which led to spurious "Unable to log query"
  errors.

Partial fix for #4047.

* Code style

* Remove unused imports.

* Fix tests.

* Remove unused import.
gianm added a commit to implydata/druid-public that referenced this issue Aug 14, 2017
@gianm
Add metrics to the native queries underpinning SQL. (apache#4561)
d4fc55e 
* Add metrics to the native queries underpinning SQL.

This is done by factoring out the metrics and request log emitting
code from QueryResource into a new QueryLifecycle class. That class
is used by both QueryResource and the SQL DruidSchema and QueryMaker.

Also fixes a couple of bugs in QueryResource:

- RequestLogLine start time was set to `TimeUnit.NANOSECONDS.toMillis(startNs)`,
  which is incorrect since absolute nanos cannot be converted to millis.
- DruidMetrics.makeRequestMetrics was called with null `query` on
  unparseable queries, which led to spurious "Unable to log query"
  errors.

Partial fix for apache#4047.

* Code style

* Remove unused imports.

* Fix tests.

* Remove unused import.
@jon-wei jon-wei modified the milestones: 0.11.0, 0.11.1 Sep 20, 2017
@gianm gianm removed this from the 0.11.1 milestone Oct 16, 2017
@gaodayue gaodayue mentioned this issue Sep 5, 2018
Closed
@stale
Copy link

stale bot commented Jun 21, 2019

This issue has been marked as stale due to 280 days of inactivity. It will be closed in 2 weeks if no further activity occurs. If this issue is still relevant, please simply write any comment. Even if closed, you can still revive the issue at any time or discuss it on the [email protected] list. Thank you for your contributions.

@stale stale bot added the stale label Jun 21, 2019
@stale
Copy link

stale bot commented Jul 5, 2019

This issue has been closed due to lack of activity. If you think that is incorrect, or the issue requires additional review, you can revive the issue at any time.

@stale stale bot closed this as completed Jul 5, 2019
@jihoonson jihoonson mentioned this issue Aug 31, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area - SQL Improvement stale
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@leventov @gianm @drcrallen @jon-wei