You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "suneet-s (via GitHub)" <gi...@apache.org> on 2023/03/09 05:44:15 UTC
[GitHub] [druid] suneet-s opened a new pull request, #13906: Report engine as a metric for sqlQuery metrics
suneet-s opened a new pull request, #13906:
URL: https://github.com/apache/druid/pull/13906
### Description
Add engine as a dimension to all sqlQuery metrics. This will make it possible for users to distinguish between query times based on the type of query that is run.
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s merged PR #13906:
URL: https://github.com/apache/druid/pull/13906
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1130511177
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
What is the correct way to get a reference to the engine? I also noticed it's available via the plannerContext.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1130546762
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
heh, the statement also has a plannerContext, so could do `plannerContext.getEngine().name()` or `stmt.plannerContext.getEngine().name()` or what you did here... probably others too 😜
Most of the other stuff looks like it is coming from `plannerContext` so that seems probably best?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1131404039
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
I chose this instead of `plannerContext` because it looks like `sqlToolbox` is never null, while `plannerContext` might be null. Also, having to a nested if to check that the engine in the plannerContext is not null, seemed like it would make the code harder to read / follow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1132624714
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
The code will crash if the toolbox is null. However, given the ambiguity introduced that class you mentioned, it is not harmful to include the check here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] paul-rogers commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "paul-rogers (via GitHub)" <gi...@apache.org>.
paul-rogers commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1131433727
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
The toolbox is never null. The engine is also never null, so no need for the `null` check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #13906: Report engine as a dimension for sqlQuery metrics
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #13906:
URL: https://github.com/apache/druid/pull/13906#discussion_r1131458319
##########
sql/src/main/java/org/apache/druid/sql/SqlExecutionReporter.java:
##########
@@ -94,6 +94,9 @@ public void emit()
metricBuilder.setDimension("id", plannerContext.getSqlQueryId());
metricBuilder.setDimension("nativeQueryIds", plannerContext.getNativeQueryIds().toString());
}
+ if (stmt.sqlToolbox.engine != null) {
+ metricBuilder.setDimension("engine", stmt.sqlToolbox.engine.name());
Review Comment:
@paul-rogers The sqlEngine in the SqlToolbox provided by the SqlStatementFactoryModule is null
https://github.com/apache/druid/blob/master/sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java#L173
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org