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.

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

ExecutedActionMetadata: Allow adding auxiliary metadata #154

Merged

Conversation

EdSchouten
Copy link
Collaborator

ExecutedActionMetadata provides a number of fields that help us to get insight in the worker that executed a build action, such as the name of the worker and some basic timing statistics.

Buildbarn ships with a small local modification to ExecutedActionMetadata, adding a 'repeated google.protobuf.Any' field. This field is used by some of Buildbarn's components to embed even more metrics into ActionResult, such as POSIX getrusage(2) information and temporary space allocation stats:

https://github.com/buildbarn/bb-remote-execution/blob/master/pkg/proto/resourceusage/resourceusage.proto

By having this data embedded in the ActionResult, people can view this information by simply inspecting a build action through bb-browser. This makes it easier for people to diagnose build failures accurately.

By using google.protobuf.Any, build clusters can expose their own metrics without requiring any further protocol extensions. It doesn't make sense to standardise those metrics, as they tend to be strongly implementation specific.

ExecutedActionMetadata provides a number of fields that help us to get
insight in the worker that executed a build action, such as the name of
the worker and some basic timing statistics.

Buildbarn ships with a small local modification to
ExecutedActionMetadata, adding a 'repeated google.protobuf.Any' field.
This field is used by some of Buildbarn's components to embed even more
metrics into ActionResult, such as POSIX getrusage(2) information and
temporary space allocation stats:

https://github.com/buildbarn/bb-remote-execution/blob/master/pkg/proto/resourceusage/resourceusage.proto

By having this data embedded in the ActionResult, people can view this
information by simply inspecting a build action through bb-browser. This
makes it easier for people to diagnose build failures accurately.

By using google.protobuf.Any, build clusters can expose their own
metrics without requiring any further protocol extensions. It doesn't
make sense to standardise those metrics, as they tend to be strongly
implementation specific.
@googlebot googlebot added the cla: yes Pull requests whose authors are covered by a CLA with Google. label Jul 21, 2020
santigl pushed a commit to santigl/remote-apis that referenced this pull request Aug 26, 2020
Copy link
Collaborator

@bergsieker bergsieker left a comment

Choose a reason for hiding this comment

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

General questions for followup from RE API monthly conversations. I'll loop back to comment more on these later.

  • Extensibility is good and desirable in general. It's possible this should be paired with some level of standardization, e.g., for POSIX-related data.

  • This message is fairly bloated and may not be very useful in the context of cache hits. We may want to consider moving it elsewhere or not returning it with cache hits.

  • Can probably approve this as-is and loop back on these questions later.

Copy link
Collaborator

@sstriker sstriker left a comment

Choose a reason for hiding this comment

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

While I prefer standardized fields for common statistics, I tend to agree with @bergsieker to not hold this up at this stage.

@sstriker sstriker merged commit aa8e718 into bazelbuild:master Oct 30, 2020
EdSchouten added a commit to buildbarn/bb-storage that referenced this pull request Oct 30, 2020
This allows us to get rid of a local patch. This one has been merged as
part of PR #154:

bazelbuild/remote-apis#154
@EdSchouten EdSchouten deleted the eschouten/20200721-auxiliary-metadata branch October 30, 2020 20:21
@EdSchouten
Copy link
Collaborator Author

Thanks for merging this @sstriker!

@bergsieker If you want I can send a followup PR to add POSIXResourceUsage to remote_execution.proto. Or do we want to add this to some kind of separate proto file as part of this repository?

@bergsieker
Copy link
Collaborator

I'm inclined to leave this as a generic field rather than breaking out POSIX data explicitly. (My comment above was a summary of discussion from the RE monthly, not my personal opinion.) So far we've managed to keep the API itself mostly free of platform-specific structures. I would anticipate that this data is generally used for logging purposes, not for active evaluation by the client, in which case I think there's not much point in overly standardizing it.

mostynb added a commit to mostynb/remote-apis that referenced this pull request Nov 28, 2020
Followup to bazelbuild#154, which forgot to update the bazel configuration.
This makes hooks/pre-commit work again.
@mostynb
Copy link
Contributor

mostynb commented Nov 28, 2020

This change is missing a dependency which broke the bindings generation, fix here: #182

sstriker pushed a commit that referenced this pull request Dec 9, 2020
* Fix go bindings generation

Followup to #154, which forgot to update the bazel configuration.
This makes hooks/pre-commit work again.

* Add a simple CI check that bindings generation works
peterebden pushed a commit to peterebden/remote-apis that referenced this pull request Dec 18, 2020
* Fix go bindings generation

Followup to bazelbuild#154, which forgot to update the bazel configuration.
This makes hooks/pre-commit work again.

* Add a simple CI check that bindings generation works
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Pull requests whose authors are covered by a CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants