ActionExecuted proto missing SpawnExec

[fzakaria]

Description of the bug:

The proto description for ActionExecuted in the BEP mentions:

message ActionExecuted {
# other stuff
  // Additional details about action execution supplied by strategies. Bazel
  // options will determine which strategy details are included when multiple
  // strategies are involved in a single action's execution.
  //
  // The default type will be `tools.proto.SpawnExec` found in `spawn.proto`.
  repeated google.protobuf.Any strategy_details = 14;
}

However even with --build_event_publish_all_actions I can not seem to find any details in the proto.

Searching the code base seems to show that it's never included in the ActionExecuted proto.

Is this by design or a mistake? I know it's in the ExecutionLog but I'm trying to parse the BEP to derive insights and having a single file is pretty handy.

Which category does this issue belong to?

Core

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

> bazel build //src:bazel-dev --build_event_publish_all_actions --build_event_binary_file=bep.binpb

Check the proto or JSON file and no action has any spawn

[fzakaria]

Is it possible to add this information to the ActionEvent, and if so would that change be welcomed?

few more findings: I only can find it published it in SpawnLogModule for the execution log.

It is emitted https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/exec/ExpandedSpawnLogContext.java#L157

I am still trying to go through the blame to see if it was purposefully removed ? There seems to be handling (including TypeDescriptor for JSON) in the BEP for SpawnExec

[tjgq]

The strategy_details field was introduced in https://github.com/bazelbuild/bazel/commit/2ddacab80af1dca3c04e74809827c47ec4bc8498 but seemingly never populated. @michaeledgar might have more information, but he's currently OOO.

In my opinion, SpawnExec is too heavyweight for the BEP (it would unacceptably increase its size and slow down the build, as is the case with the non-compact execution log). So, to the extent that we might decide to add this information to the BEP, I think we'd want to include a significantly trimmed down and/or compressed version of SpawnExec, similar to what was done in the compact execution log.

[fzakaria]

@tjgq thank you for the context.

Since actions are already pruned (except for failed ones) unless the publish all actions flag is given maybe it could be okay to include them ?

We could also include it with an additional flag but I think the build_event_publish_all_actions is quite explicit.

[michaeledgar]

As I mentioned in #19471, I ran into a bit of a can of worms trying to export this information with full fidelity, and the effort ended up sidelined.

In my opinion, SpawnExec is too heavyweight for the BEP (it would unacceptably increase its size and slow down the build, as is the case with the non-compact execution log). So, to the extent that we might decide to add this information to the BEP, I think we'd want to include a significantly trimmed down and/or compressed version of SpawnExec, similar to what was done in the compact execution log.

This feature makes the most sense in the context of the original FR, which describes longer-running builds with (presumably) a smaller number of total actions, where users might want to see the in-progress state of an individual target with action-level granularity, and know details about each action. So in this context, --build_event_publish_all_actions usage is pretty much implied, in which case your BEP is already going to be quite large.

In retrospect, for this feature to be necessary, it also implies the user can't easily split up their multi-action targets into multiple targets with a single action. If one did that, BEP would give you what you need today.

I am curious if this really is a potent use-case for many users, or if it isn't satisfactory to use BEP for target-granularity data and the profile for action-level data.

[tjgq]

I am curious if this really is a potent use-case for many users, or if it isn't satisfactory to use BEP for target-granularity data and the profile for action-level data.

Not just the trace profile, but also the execution log.

[tjgq]

Per internal discussion: we've decided not to do implement this in light of it being duplicative of the execution log.

[fzakaria]

@tjgq should the code be removed then and also stripped from the proto as a connecting issue?

My 2cents: The execution log especially the compact variant is a lot more hoops to go through to cross-reference it with the event log. It's kind of odd that all these logs sort of have different information. Consolidating them with a big UI like BuildBuddy or whatever Google has internally makes sense but doing anything locally is a pain vs. just looking at a single log file.