initial server metadata proto fields for history event #703
Conversation
|
Overall looks good to me assuming that you get consensus on this approach. Don't merge this please until there's a working implementation that verifies it. |
| repeated temporal.api.common.v1.Link links = 302; | ||
| // The event details. The type must match that in `event_type`. | ||
| // Actions metering metadata for the event. | ||
| temporal.api.server.v1.ServerMetadata server_metadata = 303; |
There was a problem hiding this comment.
I think most of the fields here could be considered server metadata, you could just inline the ActionMetadata here.
There was a problem hiding this comment.
Having a separate server metadata field would make sense if it is supposed to be applied in other places though.
There was a problem hiding this comment.
I do not think we need an entirely new proto package for this
There was a problem hiding this comment.
If we do need a new package, please use a different name: we have temporal.server.api... for internal protos which is already somewhat confusing, let's not also have temporal.api.server. Maybe temporal.api.metering or temporal.api.cloudmetering to be clear this is a cloud concept?
| repeated temporal.api.common.v1.Link links = 302; | ||
| // The event details. The type must match that in `event_type`. | ||
| // Actions metering metadata for the event. | ||
| temporal.api.server.v1.ServerMetadata server_metadata = 303; |
There was a problem hiding this comment.
Unfortunately the PR where the discussion was occurring was closed. But bringing back the discussion from #701 (comment), I do not believe we should inundate history with redundant information. If people can iterate history and count actions, they can iterate history and count (certain) events.
We should not use the burden of having to read history, which already has many cryptic elements and we provide UIs and CLIs for understanding, as justification for adding redundant data.
Also, this doesn't even solve the problem of missing data (e.g. query and heartbeat counts). History is not the place for this information IMO. We should have a more general discussion about how one can derive action count from history, but this not only doesn't solve that, it inundates everyone's history format with redundant information.
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
Why?
Breaking changes
Server PR