Add additional optional metadata fields #186
Conversation
illicitonion
requested review from
agoulti,
bergsieker,
buchgr and
ola-rozenfeld
as code owners
google-cla
bot
added
the
cla: yes
illicitonion
force-pushed
the
metadata
branch
from
6d70123
to
b92e1fc
Compare
| // A namespace within which the build was performed, for instance a repository/workspace name. | ||
| string invocation_namespace_id = 6; | ||
|
|
||
| // An identifier for the target, unique within the invocation_namespace_id, which produced this action. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
|
|
||
| // A namespace within which the build was performed, for instance a repository/workspace name. | ||
| // There are no guarantees that these are globally unique, e.g. two users may create namespaces with the same name. | ||
| string invocation_namespace_id = 6; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Totally - updated to remove that field :)
This makes it possible to see the build tool's invocation ID and version number on action and uncached action result pages. This feature will become a lot more interesting once the following API for remote-apis lands: bazelbuild/remote-apis#186 Once we have that, we can display Bazel target names, configuration IDs, etc., meaning we can finally correlate Build Event Stream events with individual actions in Buildbarn.
illicitonion
force-pushed
the
metadata
branch
from
8732e93
to
7783dd9
Compare
|
I'll merge this tomorrow morning unless someone else leaves comments. |
|
@bergsieker friendly ping? :) |
|
Please import this change as well. |
This reflects bazelbuild/remote-apis#186 Closes #13109. PiperOrigin-RevId: 368763391
This reflects bazelbuild/remote-apis#186 Closes #13109. PiperOrigin-RevId: 368763391
This reflects bazelbuild/remote-apis#186 Closes #13109. PiperOrigin-RevId: 368763391
No description provided.