https://pantsbuild.org/ logo
Join Slack
Powered by
Hello! I was thinking that it would be good for `E...
# development
r
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
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
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
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
Yeah, sorry, I meant to call it 
timing_metadata
a
I would maybe remove the 
timing
prefix - I’m sure more metadata will come up too
👍 1
r
Oh, sure 🙂
w
@red-balloon-89377: in the context of remoting there will be hundreds of these, with metrics gathered elsewhere
r
“elsewhere” meaning zipkin spans, right?
w
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
Specifically the 
WorkunitStore
w
@red-balloon-89377: and buildstats, yea
a
Related: we don’t have a common identifier across all of those places, and maybe we should…
r
Gotcha. Yeah, specifically this would be useful to tie insights from the jvm 
ExecutionGraph
to remoting/speculation.
w
@average-vr-56795: relates to @red-balloon-89377 's "which Nodes and when" ticket
r
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
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
Which I guess we already have, but may turn off for nailgun.
w
And in speculation, we can still report both spans in zipkin
r
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
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
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
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
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
Mmm, not "as long as it doesn't affect v2": rather, we don't expect 
@rules
to be rendering UI with --v2
r
Oh, gotcha, gotcha 🙂
w
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
So, what we want to avoid is moving the rendering of this information to rules.
a
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
agreed.
Open in Slack
PreviousNext
Powered by