Skip to content

Conversation

@rebello95
Copy link
Collaborator

@rebello95 rebello95 commented Jan 18, 2023

It was pointed out that this information can be helpful for tracing, logging, etc., so we are exposing it on the HTTPResponse to interceptors and HTTP client subclasses. Because the HTTP status can change based on the protocol in use, it is being exposed through a separate TracingInfo type to make this more explicit.

Note that codes from Error types that are returned by URLSession (for example, when the device is offline) are not passed through since these are not valid HTTP statuses (usually negative numbers).

It was pointed out that this information can be helpful for tracing, logging, etc., so we are exposing it on the `HTTPResponse`.

Note that codes from `Error` types that are returned by `URLSession` (for example, when the device is offline) are not passed through.
@rebello95 rebello95 requested a review from akshayjshah January 18, 2023 23:13
/// HTTP status received by the response. Note that `code` is generally preferred,
/// except for HTTP-specific use cases such as tracing.
/// Nil in cases where no response was received from the server.
public let httpStatus: Int?

Choose a reason for hiding this comment

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

Exposing HTTP status codes breaks this library's abstraction and directly exposes users to the differences between the supported RPC protocols: any code that uses httpStatus will need to change if the user switches from gRPC-Web to Connect or vice versa.

I can understand why this is useful, but it'd be nice to explicitly carve it out into a struct of protocol-specific data that isn't covered by the "switch protocols with just a config flag" promise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea this is a tough one - the HTTPResponse is only used by the HTTP client and interceptors, but those interceptors (like the ones that implement gRPC-Web and Connect) do mutate code and would not mutate httpStatus. Do you think it'd be better if we move this to a separate type within the response, something like this?

public struct HTTPResponse {
    /// The status code of the response.
    /// See https://connect.build/docs/protocol/#error-codes for more info.
    public let code: Code
    /// Response headers specified by the server.
    public let headers: Headers
    /// Body data provided by the server.
    public let message: Data?
    /// Trailers provided by the server.
    public let trailers: Trailers
    /// The accompanying error, if the request failed.
    public let error: Swift.Error?
    /// Additional raw data from the server that can be used for tracing/logging.
    public let tracingInfo: TracingInfo?

    public struct TracingInfo {
        public let httpStatus: Int
    }
}

I may be misunderstanding your suggestion

Choose a reason for hiding this comment

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

Yep, that's exactly what I had in mind! TracingInfo is fine and conveys roughly the right sense - LoggingInfo, DebugInfo, NetworkDetails, or something like that works for me too.

Whatever we name the struct, I think we ought to document that unlike the rest of the HTTPResponse fields, users should expect the debug info to change if the client switches between the Connect and gRPC-Web protocols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

@rebello95 rebello95 changed the title Add httpStatus to HTTPResponse Add tracing info with HTTP status to HTTPResponse Jan 27, 2023
@rebello95 rebello95 merged commit 4454254 into main Jan 27, 2023
@rebello95 rebello95 deleted the expose-httpstatus branch January 27, 2023 13:46
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.

4 participants