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

Embed partial ExecutedActionMetadata in ExecuteOperationMetadata #238

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

roitk
Copy link
Contributor

@roitk roitk commented Jan 13, 2023

The ExecutedActionMetadata contains a lot of interesting information about the work that was done, such as the worker the job was run on, timestamps for various execution milestones, and so on. This information is currently only available after the work is complete, in the ActionResult. However, I think some of these things would be useful to look at while the execution is running as well, especially in the context of UI tools that can show the status of ongoing work (e.g. bgd-browser).

Unfortunately, there isn't currently a place to put this in the longrunning Operation that clients get back when waiting for a job. The only metadata field is used by the ExecuteOperationMetadata message, which has a very limited set of fields. So this PR just adds an ExecutedActionMetadata field in ExecuteOperationMetadata.

@peterebden
Copy link
Contributor

I like the idea of this - it seems useful to find out where an action is executing (presumably this can be found out on the server side but this makes it easier to see it on the client).

I'm a little dubious about the Any though. It seems tricky for a client to know how to interpret it - there is prior art with the auxiliary_metadata field, but it seems like that is more likely to be both populated & read by the server side, which probably agrees about what it wants to put in there.
Maybe it will be sufficient to have some de facto options for this?

@roitk
Copy link
Contributor Author

roitk commented Jan 17, 2023

Maybe it will be sufficient to have some de facto options for this?

Any thoughts on default fields? I'd personally like (at least) the worker name and the various milestones in the ExecutedActionMetadata to show up here, which could be populated as the job passes through those stages.

@peterebden
Copy link
Contributor

Yup, I agree on those. I think ExecutedActionMetadata is fairly close, although I also agree with your points before about where it's not so ideal.
On balance maybe that's a good place to start since it already exists and we don't have to define a new message? So just something like "Servers MAY choose to populate this field with a partially complete ExecutedActionMetadata message" would give a hint on how to interpret the contents.

@roitk
Copy link
Contributor Author

roitk commented Jan 18, 2023

On balance maybe that's a good place to start since it already exists and we don't have to define a new message? So just something like "Servers MAY choose to populate this field with a partially complete ExecutedActionMetadata message" would give a hint on how to interpret the contents.

I think that's a good compromise. Updated the comment.

@roitk roitk changed the title Add support for additional operation metadata Embed partial ExecutedActionMetadata in ExecuteOperationMetadata Jan 18, 2023
@sstriker sstriker self-assigned this Feb 14, 2023
@EricBurnett
Copy link
Collaborator

This LGTM as well. @sstriker I see you assigned yourself: anything more needed here?

@sstriker
Copy link
Collaborator

This LGTM as well. @sstriker I see you assigned yourself: anything more needed here?

No, we're good to go.

@sstriker sstriker merged commit 44063d0 into bazelbuild:main Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants