Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional optional metadata fields #186

Merged
merged 3 commits into from
Mar 1, 2021
Merged

Add additional optional metadata fields #186

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b92e1fc
Add additional optional metadata fields
illicitonion Feb 5, 2021
fce5dcb
Add comment about namespace non-global-uniqueness
illicitonion Feb 15, 2021
7783dd9
Remove workspace for now
illicitonion Feb 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions build/bazel/remote/execution/v2/remote_execution.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1690,4 +1690,21 @@ message RequestMetadata {
// An identifier to tie multiple tool invocations together. For example,
// runs of foo_test, bar_test and baz_test on a post-submit of a given patch.
string correlated_invocations_id = 4;

// A one-word description of the kind of action, for example, CppCompile or GoLink.
illicitonion marked this conversation as resolved.
Show resolved Hide resolved
// There is no standard agreed set of values for this, and they are expected to vary between different client tools.
string action_mnemonic = 5;

// A namespace within which the build was performed, for instance a repository/workspace name.
illicitonion marked this conversation as resolved.
Show resolved Hide resolved
string invocation_namespace_id = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's per-invocation, putting it in the per-action metadata is not ideal. I've previously seen cases where metadata ended up being more than 50% of the network traffic, with most of it being redundant. Please don't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a reasonable concern - the same is presumably true for correlated_invocations_id - I expect that's tied to a single tool_invocation_id?

Perhaps both of these fields could be attached to GetCapabilities (either as in-line metadata in the request, or as header metadata) so that a remote execution system can record an association between them, and then they could be omitted from the per-request headers?

Copy link
Collaborator

@EdSchouten EdSchouten Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to only attach the bare minimum here, and rely on the Build Event Stream to provide all other info. More concretely, this info should be part of the metadata:

  • invocation ID
  • target name
  • configuration ID

But this info could be part of the BES:

  • correlated invocations ID
  • tool name and version

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you happen to be running a BES, I think that makes complete sense, but I'm not sure closely coupling those two systems is broadly applicable enough to have one depend on the other.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave this field out for now and check the rest in? :-D The other new items seem obviously a good idea to add to the proto.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally - updated to remove that field :)


// An identifier for the target, unique within the invocation_namespace_id, which produced this action.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what it means to be "unique within the invocation_namespace_id". I could see unique within the build (tool_invocation_id) though even that gets confusing with multiple configurations, but I'm not sure how to extend that to multiple invocations on a namespace. Maybe call out what can be inferred from this uniqueness?

(Also, this may require invocation_namespace_id be globally unique).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this is trying to get at is basically: Within one namespace, two actions with the same target were generated by the same target. (Which, if you can rely on your users' namespace_ids being unique, is a strong statement, and otherwise probably just means that within a tool_invocation_id two actions with the same target were generated by the same target). I can just remove the comment about uniqueness, if you think that would be more clear?

// No guarantees are made around how many actions may relate to a single target.
string target = 7;
illicitonion marked this conversation as resolved.
Show resolved Hide resolved

// An identifier for the configuration in which the target was built,
// e.g. for differentiating building host tools or different target platforms.
// There is no expectation that this value will have any particular structure,
// or equality across invocations, though some client tools may offer these guarantees.
string configuration_id = 8;
}