Skip to content
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

Handle encoding and decoding errors in a middleware-like extension point #522

Open
jpsim opened this issue Feb 3, 2024 · 15 comments
Open
Labels
area/runtime Affects: the runtime library. kind/enhacement Improvements to existing feature.
Milestone

Comments

@jpsim
Copy link

jpsim commented Feb 3, 2024

Question

I'd like to add telemetry to my app to report issues where an API response is missing a required key or otherwise has decoding errors. Ideally I could write middleware to achieve this, but from what I've been able to tell, middleware can't intercept decoding errors.

In a perfect world, the server handling the API requests would always return 100% spec-compliant responses, never introducing breaking changes, but mistakes happen.

How would you suggest I add this telemetry to my client application using swift-openapi-*?

@jpsim jpsim added kind/support Adopter support requests. status/triage Collecting information required to triage the issue. labels Feb 3, 2024
@czechboy0
Copy link
Collaborator

czechboy0 commented Feb 3, 2024

Hi @jpsim,

you're right that a middleware won't be the solution here, as transports and middlewares still work with generic HTTP requests and responses, and only the type-safe layer closest to your code turns them into values of your generated types.

If you want to e.g. emit metrics for specifically decoding errors, you can catch ClientError, which, as its underlyingError will have the original DecodingError thrown by JSONDecoder. I'd suggest logging it together with the causeDescription.

So, in code, you could do something like:

@main struct HelloWorldURLSessionClient {
    static func main() async throws {
        let client = Client(serverURL: URL(string: "http://localhost:8080/api")!, transport: URLSessionTransport())
        do {
            let response = try await client.getGreeting()
            print(try response.ok.body.json.message)
        } catch let error as ClientError where error.underlyingError is DecodingError {
            print("The reason for this request failing was the following decoding error: \(error)")
            // Increment your metric here.
            // Rethrow the original error.
            throw error
        }
    }
}

@jpsim
Copy link
Author

jpsim commented Feb 3, 2024

Yes, I can catch decoding errors at the call site like this, but that will lead to a lot of duplication.

This seems like a perfect fit for middleware conceptually, where other metrics and instrumentation can easily be added for all endpoints without needing the call sites to explicitly handle those cases.

Consider this a feature request to consider having centralized handling of decoding errors 🙏

And thanks for all your efforts in making this library ❤️

@czechboy0
Copy link
Collaborator

czechboy0 commented Feb 3, 2024

Thanks for the kind words 🙂

If we were to add a middleware at the suggested layer, do you have ideas of what the signature could be?

Thinking out-loud here, but if we based it on the raw HTTP one:

protocol Middleware: Sendable {
    func intercept(_ input: Sendable, operationId: String, next: (Sendable) async throws -> Sendable) async throws -> Sendable
}

The input/output values would be of the Operations.MyOperation.{Input,Output} types.

Would that be useful? I wonder if the as? casting you'd need to do in the body of it would actually be useful/acceptable.

Opinions/ideas welcome here, also cc @simonjbeaumont.

@jpsim
Copy link
Author

jpsim commented Feb 3, 2024

Could we keep the type information by using generic parameters?

func intercept<Input: Sendable, Output: Sendable>(
  input: Input,
  operationId: String,
  next: (Input) async throws -> Output
) async throws -> Output {
  // Middleware could transform or log input, output or errors here
  return try await next(input)
}

@czechboy0
Copy link
Collaborator

The Client type needs to have an array of these middlewares that all run on every request to every operation. But these Input/Output types are per-operation.

Today, we have a property var middlewares: [any ClientMiddleware] on the underlying client. In my less-than-ideal scenario, that would be var middlewares: [any Middleware], for example.

What would that array signature be in your example? How would the Client code iterate through them?

@jpsim
Copy link
Author

jpsim commented Feb 3, 2024

I think with my suggestion, you could keep var middlewares: [any ClientMiddleware] since the generic parameters are defined on the function, not the protocol, so it doesn't add any generic constraints to the protocol itself.

@czechboy0
Copy link
Collaborator

Sorry, I'm still not sure I understand how it'd actually work. Could you try to open a draft PR to the runtime library with your experimental change, and maybe that'll help me get it? 😇

jpsim added a commit to jpsim/swift-openapi-runtime that referenced this issue Feb 4, 2024
This is just a proof-of-concept, if this direction is reasonable, it
should be fleshed out to handle all decoding paths.

See apple/swift-openapi-generator#522 for
motivation.
@jpsim
Copy link
Author

jpsim commented Feb 4, 2024

Proof of concept here: apple/swift-openapi-runtime#99

I basically just took the shortest path I could find that would allow me to hook into decoding errors in a centralized place. I suspect you may want to go a different direction that may be a better fit conceptually for the project, but I'll leave that to you to decide.

@czechboy0
Copy link
Collaborator

Oh right, if you don't need the Input/Output when handling these errors, then yes we could totally do that.

A few thoughts:

  • should this cover encoding errors as well, and errors such as a missing header when it was required?
  • should the method itself throw, to allow you to handle the error and rethrow a different one (or the same one)? Maybe the method should return an error to ensure we always have something to rethrow after the handler returns.
  • should the error be ClientError, which has more useful context, or just the underlying error?

Yeah let's discuss, this is definitely an interesting direction.

@jpsim
Copy link
Author

jpsim commented Feb 4, 2024

These are no doubt important questions for you to answer as a library author. From my perspective, any answer to those questions would satisfy my need to add instrumentation and alerting to api response decoding errors.

@simonjbeaumont
Copy link
Collaborator

Oh right, if you don't need the Input/Output when handling these errors, then yes we could totally do that.

A few thoughts:

  • should this cover encoding errors as well, and errors such as a missing header when it was required?
  • should the method itself throw, to allow you to handle the error and rethrow a different one (or the same one)? Maybe the method should return an error to ensure we always have something to rethrow after the handler returns.
  • should the error be ClientError, which has more useful context, or just the underlying error?

Yeah let's discuss, this is definitely an interesting direction.

Users can already catch and unpack the error today; the complaint wasn't that it wasn't possible, but that it was tedious to wrap every call like this. For this reason, I think that allowing the user to convert to a different error doesn't add much value. Same argument goes for whether this error is wrapped or unwrapped.

IIUC, the request is to instrument this particular error flow to log or collect metrics? In that case, any hook we provide should probably be purely an optional side-effect, so a void function.

I have a couple of questions of my own:

  • If we're providing this as an optional function hook that users can provide, why make it specific to this error? Are there more places where such a handler could be beneficial.
  • From an API perspective, could we position this as a middleware still (cf. a function hook), even if it means we need to do some things behind the scenes? E.g. ErrorMiddleware(_ handler: (any Error) -> Void).
  • If it's just instrumentation that users are after, should we consider adding "actual" instrumentation to the runtime library, i.e. by depending on Swift Metrics and emitting metrics for these sorts of things with dimensions for the operationID and the kind of error?

@czechboy0
Copy link
Collaborator

All great points. If we say that this is purely a read-only action (no transforming of errors), then maybe something like this? (Naming to be bikeshed separately)

protocol ClientErrorDelegate: Sendable {
  func requestSerializationDidThrow(_ error: ClientError) async throws
  func transportOrMiddlewareDidThrow(_ error: ClientError) async throws
  func responseDeserializationDidThrow(_ error: ClientError) async throws
}

And then you could optionally provide one of these on the Configuration struct. WDYT?

@czechboy0
Copy link
Collaborator

czechboy0 commented Feb 5, 2024

We'd probably also provide a server variant:

protocol ServerErrorDelegate: Sendable {
  func requestDeserializationDidThrow(_ error: ClientError) async throws
  func transportOrMiddlewareDidThrow(_ error: ClientError) async throws // Not entirely sure how this one would work, TBD
  func responseSerializationDidThrow(_ error: ClientError) async throws
}

// Edit: Actually, if this is Client/Server, we'd probably just add it as another parameter to the Client initializer/registerHandlers call. Configuration only has things that are the same for both today.

@jpsim
Copy link
Author

jpsim commented Feb 5, 2024

If it's just instrumentation that users are after, should we consider adding "actual" instrumentation to the runtime library, i.e. by depending on Swift Metrics and emitting metrics for these sorts of things with dimensions for the operationID and the kind of error?

  1. Comprehensive metrics would be great, but much broader scope compared to my near-term needs
  2. Ideally comprehensive metrics would be pluggable like ClientTransport currently is to:
    a. avoid the cost of the swift-metrics dependency for users who don't need it
    b. allow for alternative metrics implementations

Also I consider fine-grained metrics (number of requests, failure #s, timing data histograms, etc) to be fairly distinct from and very differently actionable compared to decoding errors where a server is returning non-conformant responses. So I can see some benefit in having a unified metrics solution but that's not necessary for this level of instrumentation.

@czechboy0 czechboy0 changed the title Is it possible to write middleware to instrument decoding errors? Handle encoding and decoding errors in a middleware-like extension point Apr 5, 2024
@czechboy0 czechboy0 added area/runtime Affects: the runtime library. kind/enhacement Improvements to existing feature. and removed status/triage Collecting information required to triage the issue. kind/support Adopter support requests. labels Apr 16, 2024
@czechboy0 czechboy0 added this to the Post-1.0 milestone Apr 16, 2024
@czechboy0
Copy link
Collaborator

Discussion happening on the runtime PR: apple/swift-openapi-runtime#99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/runtime Affects: the runtime library. kind/enhacement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants