Skip to content

generated clients sketch#315

Closed
ahl wants to merge 10 commits into
mainfrom
openapi
Closed

generated clients sketch#315
ahl wants to merge 10 commits into
mainfrom
openapi

Conversation

@ahl

@ahl ahl commented Oct 16, 2021

Copy link
Copy Markdown
Contributor

This is an example of a trivial generated client. I'm looking for early feedback. Some things I'd note:

  • progenitor forces consumers to pull in a few dependencies. Some of them seem fine (e.g. it can require chrono and uuid) but I think it would be nice to wrap these up more elegantly

  • I don't love the use of anyhow here. I think the client could be more precise about its errors

  • The use of reqwest seems permissible, but I'd love to hear feedback there

  • Note that agent.request_share becomes agent.api_request_share because that's what we call the actual endpoint function. I think we should just drop prefixes like "api" in actual endpoint functions, but we could alternatively 1. add an option to the endpoint macro to change the operationId or 2. maybe have some way to tell dropshot to drop prefixes generally, but I have no idea what that would look like.

  • We lose logging. This seems problematic.

@ahl ahl requested review from davepacheco and smklein October 16, 2021 01:18
@david-crespo

Copy link
Copy Markdown
Contributor

Very cool. On 4, I like changing the function name as well. We can wait and see if we have enough places we'd want to override the operation ID before adding that.

@jclulow

jclulow commented Oct 16, 2021

Copy link
Copy Markdown
Collaborator

The anyhow use in Progenitor is definitely just because I was lazy at the time. We should replace it with a thiserror thing that at a minimum allows one to extract the status code from an error.

@andrewjstone andrewjstone left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slick

@smklein

smklein commented Oct 18, 2021

Copy link
Copy Markdown
Collaborator

I'm echoing other comments, but:

  • Removing API prefixes from endpoints sounds good - I don't think an ability for progenitor to "rename them" in the client bindings is at all necessary.
  • Anyhow -> thiserror in libs is always good.

On the note of logging - couldn't we possibly generate this too? I know @bnaecker has been working on integrating dtrace probes into the server endpoints; I wonder if there's a way we could have some automated goop such that on each {client, server} invocation, we call {dtrace provider functions, slog logging functions}.

@ahl

ahl commented Oct 18, 2021

Copy link
Copy Markdown
Contributor Author

On the note of logging - couldn't we possibly generate this too? I know @bnaecker has been working on integrating dtrace probes into the server endpoints; I wonder if there's a way we could have some automated goop such that on each {client, server} invocation, we call {dtrace provider functions, slog logging functions}.

I've been thinking more about the code we generate, how consumers might customize that code generation, and how consumers might customize at the more generic HTTP client layer rather than at the API. For example, this uses reqwest today, but one could imagine a generated client using some trait instead. We could potentially put logging and tracing at that more generic layer rather than in the generated API code.

@davepacheco

Copy link
Copy Markdown
Collaborator

This all sounds good.

Regarding using reqwest as a client: @ahl and I discussed this offline but I figured I'd put this here, too. We're eventually going to want some sophisticated management of the underlying TCP connections. Most out-of-the-box clients (including reqwest, as far as I can tell) include a very simple pool of connections that I think will fall apart for us at scale. More sophisticated clients look more like what I described here. I think we should assume we're going to need the sophisticated thing eventually, though maybe not for v1.

This could influence the interface that these clients provide. But @ahl's and my conclusion is that the change described here is certainly a step in the right direction, and we can evolve it as needed when we figure out how we want to expose better connection management. Unless folks disagree, I think we should probably punt on that part for now.

@ahl

ahl commented Oct 29, 2021

Copy link
Copy Markdown
Contributor Author

I added support for logging. @davepacheco and @jclulow I'd particularly appreciate your feedback:

https://github.com/oxidecomputer/omicron/blob/1d36fcfd85a148d257247a1f6e55506250482e33/sled-agent/src/bootstrap/client.rs

generate_api!(
    "../openapi/bootstrap-agent.json",
    slog::Logger,
    |log: &slog::Logger, request: &reqwest::Request| {
        debug!(log, "client request";
            "method" => %request.method(),
            "uri" => %request.url(),
            "body" => ?&request.body(),
        );
    },
    |log: &slog::Logger, result: &Result<_, _>| {
        debug!(log, "client response"; "result" => ?result);
    },
);

In short, you can optionally specify a type, a pre-request hook, and a post-request hook. This would let us do USDT probes pretty nicely as well.

@davepacheco

Copy link
Copy Markdown
Collaborator

Looks pretty cool!

@ahl ahl force-pushed the openapi branch 2 times, most recently from 38c1a09 to 03455b4 Compare November 3, 2021 19:20
@ahl ahl changed the title generated client sketch generate clients from openapi Nov 7, 2021
@ahl ahl changed the title generate clients from openapi generated clients sketch Nov 7, 2021
@ahl ahl closed this Nov 7, 2021
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.

6 participants