Add query plans to profile output #128828
Conversation
idegtiarenko
added
>non-issue
Team:Analytics
| return b; | ||
| }), | ||
| ChunkedToXContentHelper.array("drivers", profile.drivers.iterator(), params), | ||
| ChunkedToXContentHelper.array("plans", profile.plans.iterator()), |
There was a problem hiding this comment.
This adds following section:
"plans": [
{
"description": "data",
"cluster_name": "runTask",
"node_name": "runTask-0",
"plan_tree": "ExchangeSinkExec[[language_code{f}#2],false]\n\\_ProjectExec[[language_code{f}#2]]\n \\_FieldExtractExec[language_code{f}#2]<[],[]>\n \\_EsQueryExec[data], indexMode[standard], query[][_doc{f}#3], limit[1000], sort[] estimatedRowSize[12]"
},
{
"description": "node_reduce",
"cluster_name": "runTask",
"node_name": "runTask-0",
"plan_tree": "ExchangeSinkExec[[language_code{f}#2],false]\n\\_ExchangeSourceExec[[language_code{f}#2],false]"
},
{
"description": "data",
"cluster_name": "runTask",
"node_name": "runTask-1",
"plan_tree": "ExchangeSinkExec[[language_code{f}#1],false]\n\\_ProjectExec[[language_code{f}#1]]\n \\_FieldExtractExec[language_code{f}#1]<[],[]>\n \\_EsQueryExec[data], indexMode[standard], query[][_doc{f}#2], limit[1000], sort[] estimatedRowSize[12]"
},
{
"description": "node_reduce",
"cluster_name": "runTask",
"node_name": "runTask-1",
"plan_tree": "ExchangeSinkExec[[language_code{f}#1],false]\n\\_LimitExec[1000[INTEGER],8]\n \\_ExchangeSourceExec[[language_code{f}#1],false]"
},
{
"description": "final",
"cluster_name": "runTask",
"node_name": "runTask-0",
"plan_tree": "OutputExec[org.elasticsearch.xpack.esql.plugin.ComputeService$$Lambda/0x000000000c464e08@35b2b181]\n\\_LimitExec[1000[INTEGER],8]\n \\_ExchangeSourceExec[[language_code{f}#2],false]"
}
]
json is not particularly great when displaying multi-line strings. I wonder if there is any trick that could help us make tree a bit more readable? May be display array of lines instead of songle line with breaks?
There was a problem hiding this comment.
I'd be fine with breaking on \n into an array. It's easier to read.
I'd also be fine keeping the big string. It's quite honest about what's really there.
I suppose the best thing would be to build some kind of object tree structure. The plan tree rendering code already does some of that - but it's optimized for string-y-ness. It'll, like, discard long stuff too. And we don't need that in this json response. At least, not for readability. It'd be bad to have a 40mb plan. But I don't think this is worth doing in this PR. It's a bunch more work and either the array of lines or the giant string is good.
There was a problem hiding this comment.
The plan-tree is actually an ASCII-art rendering of a tree structure. We should instead export that tree as a JSON tree (with parent-child nesting). I was planning to do this based on feedback from the work I did for query plan visualization. Costin pointed out that there was precedence in the QL code-base for exporting to graphviz. We do not need to go and support graphviz, but leave something as specific as that to external tools. We should at least print the plan tree as JSON instead of ascii-art. This would be a good improvement.
# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
nik9000
added
auto-backport
There was a problem hiding this comment.
LGTM.
Two important things to think about:
- I think you should backport this, even though it'll take a while. It'll be nice to have in 8.19 given how long it'll live.
- Does this appear in the
taskoutput? Can you add it? I think it'd be useful to have there.
If this is already in the task can you add something to EsqlActionTaskIT? If it's not in the task, I'm happy for you merge this as is and add to the task later if you have time. Or add it in this PR. Either is fine by me.
| @@ -281,6 +281,7 @@ static TransportVersion def(int id) { | |||
| public static final TransportVersion IDP_CUSTOM_SAML_ATTRIBUTES = def(9_087_0_00); | |||
| public static final TransportVersion JOIN_ON_ALIASES = def(9_088_0_00); | |||
| public static final TransportVersion ILM_ADD_SKIP_SETTING = def(9_089_0_00); | |||
| public static final TransportVersion ESQL_PROFILE_INCLUDE_PLAN = def(9_090_0_00); | |||
There was a problem hiding this comment.
I had said to auto-backport this. but it needs a transport version change. I should have know it does.
I think it's still worth it. Imagine someone giving us an 8.19 profile a year from now - we'll be happy that you added the plan....
| @@ -286,13 +287,19 @@ private Iterator<ToXContent> profileRenderer(ToXContent.Params params) { | |||
| if (profile == null) { | |||
| return Collections.emptyIterator(); | |||
| } | |||
| return Iterators.concat(ChunkedToXContentHelper.startObject("profile"), ChunkedToXContentHelper.chunk((b, p) -> { | |||
There was a problem hiding this comment.
I wonder if we should do like:
List<Iterator> all = new ArrayList<>();
all.add(ChunkedToXContentHelper.startObject("profile"));
if (executionInfo != null) {
all.add(ChunkedToXContentHelper.chunk((b, p) -> b.field("query", executionInfo.overallTimeSpan()),field("planning", executionInfo.planningTimeSpan());
}
...
That keeps everything on one line nicely and means expansions don't make big changes to the layout. And the ArrayList isn't a big allocation - just a bunch of pointers in it.
| return b; | ||
| }), | ||
| ChunkedToXContentHelper.array("drivers", profile.drivers.iterator(), params), | ||
| ChunkedToXContentHelper.array("plans", profile.plans.iterator()), |
There was a problem hiding this comment.
I'd be fine with breaking on \n into an array. It's easier to read.
I'd also be fine keeping the big string. It's quite honest about what's really there.
I suppose the best thing would be to build some kind of object tree structure. The plan tree rendering code already does some of that - but it's optimized for string-y-ness. It'll, like, discard long stuff too. And we don't need that in this json response. At least, not for readability. It'd be bad to have a 40mb plan. But I don't think this is worth doing in this PR. It's a bunch more work and either the array of lines or the giant string is good.
# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlQueryResponse.java
💔 Backport failed
You can use sqren/backport to manually backport by running |
|
Backport depends on #125631 |
Done. |
(cherry picked from commit 56d5009)
(cherry picked from commit 56d5009)
Query plan might be essential in order to understand how query is executed.
In order to see it one needs to enable TRACE logging in number of classes.
This might not be practical or even possible in a cluster running lots of queries.
This change adds output with executed plans to the query profile output, please see below:
Sample profile output