feat: capture more metrics in distributed_query - #1353
Conversation
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
milenkovicm
left a comment
milenkovicm
left a comment
There was a problem hiding this comment.
i think this makes sense @PhVHoang thanks a lot!
One minor comment to investigate
| .max_encoding_message_size(max_message_size) | ||
| .max_decoding_message_size(max_message_size); | ||
|
|
||
| let query_submit_time = std::time::Instant::now(); |
There was a problem hiding this comment.
just wonder if SuccessfulJob::queued_at could be used instead of this one? I'm not quite sure, wdyt?
There was a problem hiding this comment.
Thanks @milenkovicm ,
I think this is a great catch. Using queued_at would be more accurate since it’s a server-side timestamp set by the scheduler when the job is queued. This avoids client-side clock skew and ensures the measurement doesn’t include any network latency.
Let me fix it.
Signed-off-by: Hoang Pham <a2k40pbc@gmail.com>
| max_message_size: usize, | ||
| grpc_config: GrpcClientConfig, | ||
| metrics: Arc<ExecutionPlanMetricsSet>, | ||
| partition: usize, |
There was a problem hiding this comment.
looks like there is single partition as the result of this exec, so not sure if we need partition here or we could use 0 later?
There was a problem hiding this comment.
Let's leave it as it is for now
2 participants
Which issue does this PR close?
Closes #1252 .
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?