-
Notifications
You must be signed in to change notification settings - Fork 201
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
Serialize RPC deadline as a Duration. #367
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Duration was previously serialized as SystemTime. However, absolute times run into problems with clock skew: if the remote machine's clock is too far in the future, the RPC deadline will be exceeded before request processing can begin. Conversely, if the remote machine's clock is too far in the past, the RPC deadline will not be enforced. By converting the absolute deadline to a relative duration, clock skew is no longer relevant, as the remote machine will convert the deadline into a time relative to its own clock. This mirrors how the gRPC HTTP2 protocol includes a Timeout in the request headers [0] but the SDK uses timestamps [1]. Keeping the absolute time in the core APIs maintains all the benefits of today, namely, natural deadline propagation between RPC hops when using the current context. This serialization strategy means that, generally, the remote machine's deadline will be slightly in the future compared to the local machine. Depending on network transfer latencies, this could be microseconds to milliseconds, or worse in the worst case. Because the deadline is not intended for high-precision scenarios, I don't view this is as problematic. Because this change only affects the serialization layer, local transports that bypass serialization are not affected. [0] https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md [1] https://grpc.io/blog/deadlines/#setting-a-deadline
Using the serialization layer to hide the conversion is pretty neat. Thanks for fixing this! |
Thanks for raising this issue and providing prompt feedback, @ditsing ! |
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 30, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 30, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 30, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 31, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 31, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
mislavn
added a commit
to mislavn/tarpc
that referenced
this pull request
Jan 31, 2024
This continues on the google#367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit Tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry span's. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
tikue
pushed a commit
that referenced
this pull request
Feb 1, 2024
This continues on the #367 pull request. Tarpc should use relative time, especially since RPC serialization no longer uses SystemTime but Duration. With this commit, tarpc uses Instant time and on the edges converts Instant time to SystemTime for the Opentelemetry spans. Signed-off-by: Mislav Novakovic <mislav.novakovic@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Duration was previously serialized as
SystemTime
. However, absolute times run into problems with clock skew: if the remote machine's clock is too far in the future, the RPC deadline will be exceeded before request processing can begin. Conversely, if the remote machine's clock is too far in the past, the RPC deadline will not be enforced.By converting the absolute deadline to a relative duration, clock skew is no longer relevant, as the remote machine will convert the deadline into a time relative to its own clock. This mirrors how the gRPC HTTP2 protocol includes a
Timeout
in the request headers but the SDK uses timestamps. Keeping the absolute time in the core APIs maintains all the benefits of today, namely, natural deadline propagation between RPC hops when using the current context.This serialization strategy means that, generally, the remote machine's deadline will be slightly in the future compared to the local machine's deadline. Depending on network transfer latencies, this could be microseconds to milliseconds, or worse. Because the deadline is not intended for high-precision scenarios, I don't view this is as problematic.
Because this change only affects the serialization layer, local transports that bypass serialization are not affected.
Jaeger screenshot showing change in deadline between client and server:
Fixes #366