Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Feb 2, 2024

Motivation:

When resoling addresses, gRPC clients can discover configuration to use
published by service maintainers. This is in the form of 'service
config' and typically encoded as JSON. However the structure of the
config is a translation of the service config protobuf message using the
standard proto to JSON mappings.

This change expands on the existing MethodConfiguration to allow it to
be decoded from JSON and adds a ServiceConfiguration which can also be
decoded from JSON.

Modifications:

  • Add additional properties to MethodConfiguration and make it
    Codabale
  • Add ServiceConfiguration and make it Codabale
  • Add a RuntimeError
  • Add the service_config.proto message and its dependencies and
    generate these messages into the test module.

Result:

Service config and method config are more fully featured

@glbrntt
Copy link
Collaborator Author

glbrntt commented Feb 2, 2024

This change is in two commits:

  • the 'real' work in 76b5069
  • update proto script and generate in d7aade2

@glbrntt glbrntt requested a review from gjcairo February 2, 2024 15:33
@glbrntt glbrntt added the version/v2 Relates to v2 label Feb 2, 2024
Motivation:

When resoling addresses, gRPC clients can discover configuration to use
published by service maintainers. This is in the form of 'service
config' and typically encoded as JSON. However the structure of the
config is a translation of the service config protobuf message using the
standard proto to JSON mappings.

This change expands on the existing `MethodConfiguration` to allow it to
be decoded from JSON and adds a `ServiceConfiguration` which can also be
decoded from JSON.

Modifications:

- Add additional properties to `MethodConfiguration` and make it
  `Codabale`
- Add `ServiceConfiguration` and make it `Codabale`
- Add a `RuntimeError`
- Add the `service_config.proto` message and its dependencies and
  generate these messages into the test module.

Result:

Service config and method config are more fully featured
@@ -0,0 +1,676 @@
/*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't actually a new file, it got moved but changed quite substantially, before the configuration only included the timeout and execution policy (i.e. retry/hedging).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes also have a bit of an impact on MethodConfigurations but I'll do those in a separate PR.

///
/// In contrast to ``RPCError``, the ``RuntimeError`` represents errors which happen at a scope
/// wider than an individual RPC. For example, passing invalid configuration values.
public struct RuntimeError: Error, Hashable, @unchecked Sendable {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might make sense to replace the ClientError and ServerError with this. They all have the same structure but differ only in error codes. WDYT @gjcairo?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm my first guess would be it makes sense, but I do wonder if it would be useful from a user's perspective to be able to differentiate between client and server errors beyond the error codes. I'm thinking something like:

func wrapAndRecordMetrics(someClosure: () throws -> Void) {
  do {
    try someClosure()
  } catch is ServerError {
    // increase server error metric counter
  } catch is ClientError {
    // increase client error metric counter
  } catch {
    throw error
  }
}

But I'm not sure how realistic something like this would be, and I can't come up with any other obvious scenarios, so maybe it would make sense to replace them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to reply to this one earlier: I don't know if that's useful because we now a third RuntimeError which can be thrown by either client or server so this sort of metric recording fails to be useful anyway.

Copy link
Collaborator

@gjcairo gjcairo left a comment

Choose a reason for hiding this comment

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

LGTM overall! Left some very small nits/questions.

/// The maximum allowed payload size in bytes for an individual message.
///
/// If a client attempts to send an object larger than this value, it will not be sent and the
/// client will see a error. Note that 0 is a valid value, meaning that the request message
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: s/a error/an error

///
/// In contrast to ``RPCError``, the ``RuntimeError`` represents errors which happen at a scope
/// wider than an individual RPC. For example, passing invalid configuration values.
public struct RuntimeError: Error, Hashable, @unchecked Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm my first guess would be it makes sense, but I do wonder if it would be useful from a user's perspective to be able to differentiate between client and server errors beyond the error codes. I'm thinking something like:

func wrapAndRecordMetrics(someClosure: () throws -> Void) {
  do {
    try someClosure()
  } catch is ServerError {
    // increase server error metric counter
  } catch is ClientError {
    // increase client error metric counter
  } catch {
    throw error
  }
}

But I'm not sure how realistic something like this would be, and I can't come up with any other obvious scenarios, so maybe it would make sense to replace them.

@glbrntt glbrntt requested a review from gjcairo February 6, 2024 11:27
@glbrntt glbrntt enabled auto-merge (squash) February 6, 2024 14:04
@glbrntt glbrntt merged commit 0a7f75f into grpc:main Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

version/v2 Relates to v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants