search

twitchtv / twirp

A simple RPC framework with protobuf service definitions
https://twitchtv.github.io/twirp/docs/intro.html
Apache License 2.0
7.14k stars 327 forks source link

Varying Success Status Codes #378

Closed AlexCuse closed 1 year ago

AlexCuse commented 1 year ago

Running into an issue where we return a partial response from a certain RPC when the user has logged in but not completed MFA. We return the response and client reacts appropriately to data in it but it would be really nice for our monitoring if we could pick up the status code instead of adding a custom metric to detect this scenario.

One potential solution I see here is a special twirp.StatusCodeError - this could be returned with a response from handlers to vary the http status while still returning success responses. Codes could be limited to 2xx range if we want.

I'm happy to work on a PR for this just wanted to check if there was another facility I'm missing to do the same or a technical reason this hasn't been done yet before I dive in.

swiftyspiffy commented 1 year ago

Hi. Apologies for the delay on a response.

We discussed this and came to the conclusion that because Twirp is not just a library at this point, but also a protocol that expects a status 200 for success, that making this change would not be easy. I think the custom metric is the correct approach at this point.

AlexCuse commented 1 year ago

Thanks makes sense. A bug in something like this could cause some considerable pain. We only have a handful of spots we need it . Appreciate the detailed reply!