https://pantsbuild.org/ logo
Powered by
r

red-balloon-89377

09/19/2019, 1:28 PM
Hello! I was thinking that it would be good for 
ExecutionProcessRequests
to be able to return timing data as an extra field. This would probably take the form of 
ExecuteProcessResponse
having an extra field, similar to 
execution_attempts
, named something like 
timing_data
. This data would be a map, probably, and it would have the shape of 
{"local": "200ms", "remote": "100ms"}
. It would be transmitted across the FFI boundary, but not cached in the V2 Process Execution Cache or the Node graph. Does it sound “super useful, let’s do it”, or “let’s don’t edit the EPR API if we don’t need to”?
h

hundreds-father-404

09/19/2019, 1:32 PM
That sounds generally valuable to me as we now use V2 for the majority of CI and with @hundreds-breakfast-49010’s work will be using V2 for even more goals. However, I don’t think that’s a big priority because I haven’t seen performance be an issue. I think UX could use the engineering time much more, such as adding support for passthru args
👍 1
r

red-balloon-89377

09/19/2019, 2:27 PM
So, my not-so-secret desire here is that it would lead to good insights on remoting performance, allowing us to tune the model further if we need to 🙂
a

average-vr-56795

09/19/2019, 2:34 PM
I would probably call it 
metadata
- we’ll need to make sure it doesn’t factor into cache keys, but I’ve also been thinking about the same thing 😄
r

red-balloon-89377

09/19/2019, 2:48 PM
Yeah, sorry, I meant to call it 
timing_metadata
a

average-vr-56795

09/19/2019, 2:49 PM
I would maybe remove the 
timing
prefix - I’m sure more metadata will come up too
👍 1
r

red-balloon-89377

09/19/2019, 2:49 PM
Oh, sure 🙂
w

witty-crayon-22786

09/19/2019, 3:04 PM
@red-balloon-89377: in the context of remoting there will be hundreds of these, with metrics gathered elsewhere
r

red-balloon-89377

09/19/2019, 3:04 PM
“elsewhere” meaning zipkin spans, right?
w

witty-crayon-22786

09/19/2019, 3:04 PM
So the reason to do this would probably need to be something like: "I think I need it to render UI or log messages"
r

red-balloon-89377

09/19/2019, 3:05 PM
Specifically the 
WorkunitStore
w

witty-crayon-22786

09/19/2019, 3:05 PM
@red-balloon-89377: and buildstats, yea
a

average-vr-56795

09/19/2019, 3:05 PM
Related: we don’t have a common identifier across all of those places, and maybe we should…
r

red-balloon-89377

09/19/2019, 3:06 PM
Gotcha. Yeah, specifically this would be useful to tie insights from the jvm 
ExecutionGraph
to remoting/speculation.
w

witty-crayon-22786

09/19/2019, 3:06 PM
@average-vr-56795: relates to @red-balloon-89377 's "which Nodes and when" ticket
r

red-balloon-89377

09/19/2019, 3:07 PM
It’s not vital, because even in the presence of speculation we can just report how much the EPR took, but that would give us the 
max(remote, local)
instead of the 
min(remote, local)
unless we implement cancellation for local nodes.
a

average-vr-56795

09/19/2019, 3:07 PM
I wouldn’t object to a non-cache-affecting 
include_metadata
boolean on 
EPR
so that you could grab metadata where you want it…
r

red-balloon-89377

09/19/2019, 3:07 PM
Which I guess we already have, but may turn off for nailgun.
w

witty-crayon-22786

09/19/2019, 3:09 PM
And in speculation, we can still report both spans in zipkin
r

red-balloon-89377

09/19/2019, 3:14 PM
Yeah, it makes critical path calculations in 
ExecutionGraph
a bit imprecise, but only in the boundary case
I guess we can live without it for now, and see if we need it.
w

witty-crayon-22786

09/19/2019, 3:16 PM
Yea, I'm not morally opposed. Just pointing out adjacent stuff
maybe it's great from a UX perspective to summarize "whee, you used remoting for this one"
r

red-balloon-89377

09/19/2019, 3:21 PM
Yeah, that would be awesome, I’m just not sure that this is the way to do it, because we would only get the metadata once we have finished the request, so we can’t print things like:
Copy code
Queued - Local ():
  zinc(...)
  zinc(...)
Queued - Remote ():
  zinc(...)
  ....
Unstarted:
But that fundamentally opposes speculation.
The rest of the UX things we can print from Rust, we don’t have to go through the FFI boundary
w

witty-crayon-22786

09/19/2019, 3:23 PM
Yea. I think we should view the v1 UI here as temporary, and think about how we want this in v2
Again, that might not be in conflict with exposing this in a non-committal way: up to you
r

red-balloon-89377

09/19/2019, 3:26 PM
Okay, cool 🙂 The conclusion I have from this thread is “if we need it it’s okay, as long as it doesn’t affect the V2 UX”. I’m not sure we need it right now, because it depends on how big the boundary case is for local vs remote speculation, but it’s easy enough to implement
If that’s fair, I’ll open an issue
w

witty-crayon-22786

09/19/2019, 3:28 PM
Mmm, not "as long as it doesn't affect v2": rather, we don't expect 
@rules
to be rendering UI with --v2
r

red-balloon-89377

09/19/2019, 3:29 PM
Oh, gotcha, gotcha 🙂
w

witty-crayon-22786

09/19/2019, 3:29 PM
So we should have some other way there. If other rules have the information but aren't expected to use it, that's ok
r

red-balloon-89377

09/19/2019, 3:30 PM
So, what we want to avoid is moving the rendering of this information to rules.
a

aloof-angle-91616

09/19/2019, 6:32 PM
well, most of the things the v2 engine has now are cleaner versions of things that were hacky at first
so we'd like to decouple it here but that's an aspiration until it's built
👍 3
w

witty-crayon-22786

09/19/2019, 6:41 PM
agreed.
Powered by