Add wire tracer capabilities to reference client #768
Conversation
| Trailers []*v1.Header | ||
| // The actual JSON observed on the wire in case of an error from a Connect server. | ||
| // This will only be non-nil if the protocol is Connect and an error occurred. | ||
| ConnectErrorRaw *structpb.Struct |
There was a problem hiding this comment.
Aside: I got another one we'll want to capture. Not that you need to add it here; I can come back and add it soon.
But we will also want to capture the raw trailers in a gRPC-Web end-stream message. Because we'll want to validate that they were all lower-case on the wire.
| "connectrpc.com/conformance/internal/tracer" | ||
| ) | ||
|
|
||
| type wireInterceptor struct { |
There was a problem hiding this comment.
nit: I think it makes sense to put this in the same file as the other tracing stuff.
There was a problem hiding this comment.
I originally had it that way and then felt uneasy about that file containing so many various types and functions. I moved them back though. Let me know if it seems off.
| // Any HTTP trailers observed after the response body. These do NOT | ||
| // include trailers that conveyed via the body, as done in the gRPC-Web | ||
| // and Connect streaming protocols. | ||
| repeated Header actual_http_trailers = 3; |
There was a problem hiding this comment.
Consider moving the feedback field from ClientCompatResponse to here. Then you could also put any error message from trying to compute wire details into that field.
There was a problem hiding this comment.
In that case then, we should probably rename WireDetails to something else since it no longer is just details about the wire. Unless we consider the feedback a 'wire' detail. But, maybe something to reflect it's a reference-client only field i.e. ReferenceClientMetadata or something.
There was a problem hiding this comment.
I think the feedback would mostly be about wire details, so it would be fine to keep the name.
I don't actually have any examples of how we'd use and added it just for symmetry with the server. The server currently just uses it to complain if the incoming request doesn't have the expected properties (like HTTP version, protocol, codec, compression, etc). Technically, those are really wire details. Semantics are totally handled via the normal assertions in the test runner that compare the expected and actual responses.
Previously, computing the expected response would add an expected query parameter for the `message` param, with an encoded value. But the problem is that not all clients would necessarily encode the message in exactly the same way, and that's okay. Some will choose to base64-encode (which is usually more efficient with binary payloads). Also, there's no _canonical_ protobuf binary format, so there could be subtle differences in the byte output, even for equivalent messages. Finally, there's no _canonical_ encoding for compressed payloads -- different default compression settings or other subtle differences in implementation can lead to more than one valid compressed encoding of the same data.
This piggybacks off of the tracer framework to add wire tracing to the reference client. Broadly, this implements the
Collectorinterface and intercepts theCompletefunction invocation to sniff the built trace and acquire wire details.The details tracked are:
Note that the tests to verify raw error details trailers are not yet in place to keep the complexity of this PR lower. They will be added in a follow-up PR.