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

[HTTP] Versioned API browser client designs #152194

Closed

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 27, 2023

Summary

First pass at adding a versioned client as the counterpart to our versioned router: #151596

To reviewers

  • Is the current iteration to simplistic? Should we try to be more clever in any way?
  • Will this cause issues for any of our existing patterns? The idea is that consumers will instantiate an HTTP client paramaterised with a version and pass this all around their code. If they want to use multiple versions they will need to create another client OR we can let them pass a version at the call site. WDYT?

@jloleysens jloleysens added Feature:http Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes Epic:VersionedAPIs Kibana Versioned APIs v8.8.0 labels Feb 27, 2023
@jloleysens jloleysens self-assigned this Feb 27, 2023
@jloleysens jloleysens changed the title [HTTP] Versioned API router designs [HTTP] Versioned API browser client designs Feb 27, 2023
Comment on lines +9 to +20
import { VersionHTTPToolkit } from './version_http_toolkit';

const vtk = {} as unknown as VersionHTTPToolkit;
const client = vtk.createClient({ version: '1' });

// Now we just use client as usual.

interface FooResponse {
foo: string;
}

client.get<FooResponse>('/api/foo');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current iteration is very threadbare. It just adds a little bit of state (the version) around the HTTP client.

Let me know if there is a better approach 🙏🏻

@jloleysens jloleysens marked this pull request as ready for review March 6, 2023 10:07
@jloleysens jloleysens requested a review from a team as a code owner March 6, 2023 10:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

/** @experimental */
export interface VersionHTTPToolkit {
/**
* Create a new HTTP client for a specific version.
Copy link
Contributor

@TinaHeiligers TinaHeiligers Mar 6, 2023

Choose a reason for hiding this comment

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

Having to create a new client for every version seems like a lot of overhead and we'd end up with a lot of different clients that could get confusing.

I'd guess that ideally, we'd use the same base HTTP client with an option to specify the version.
OTOH, we'd need to keep track of which version to use per API.
I'd need to think about the implications.

import { VersionHTTPToolkit } from './version_http_toolkit';

const vtk = {} as unknown as VersionHTTPToolkit;
const client = vtk.createClient({ version: '1' });
Copy link
Contributor

@pgayvallet pgayvallet Mar 7, 2023

Choose a reason for hiding this comment

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

I share @TinaHeiligers's thoughts:

  1. My gut feeling would be that specifying the version per api call makes more sense than doing it when instantiating the client.

  2. If we do go with specifying the version per-api-call, is there any value having a dedicated client compared to the existing http service?

@jloleysens
Copy link
Contributor Author

jloleysens commented Mar 8, 2023

Thanks @pgayvallet @TinaHeiligers for sharing your thoughts!

Having to create a new client for every version seems like a lot of overhead and we'd end up with a lot of different clients that could get confusing.

My assumption is that browser/UI will, for the most part, 99% interact with one version. Maybe two versions. Assuming, again, UI developers will almost always want the latest functionality from the server at dev time.

If that is the case, my reasoning is that specifying a version per client instance will enable them to avoid refactoring all client calls to the new version once it is available. They just change it when instantiating.

That being said, it could be trivial to create a "wrapped" client scoped to a version if version is specified at call-site. I am happy to go with the less presumptuous approach for a start (specify per api-call).

If we do go with specifying the version per-api-call, is there any value having a dedicated client compared to the existing http service?

Yes, the value would be hiding versioning implementation details from consumers. Perhaps we do not need a layer over HTTP client though. We could just accept version as a parameter for all calls and map that to header/path/whatever for clients.

Looking into 🔮 here, maybe it could look like:

// server-side
router.versioned.post(...).addVersion({ version: '1', ... }, (ctx, req, res) => { ... });
// client-side
http.post('/api/cool-endpoint', { version: '1' });

What do y'all think?

@jloleysens jloleysens closed this Mar 8, 2023
@jloleysens
Copy link
Contributor Author

Oops!

@jloleysens jloleysens reopened this Mar 8, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 505 507 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jloleysens

@rudolf
Copy link
Contributor

rudolf commented Mar 8, 2023

My assumption is that browser/UI will, for the most part, 99% interact with one version. Maybe two versions. Assuming, again, UI developers will almost always want the latest functionality from the server at dev time.

This makes sense for public APIs. A plugin will usually only integrate with it's own domain and we should keep the code up to date so it always consumes the current or next version.

For internal APIs I can see us possibly diverging from this but these are just rough initial thoughts. If dashboard makes a breaking change then maybe we don't want to bump the API version for all analytics APIs? Having very granular versioning is confusing for external consumers, but internally it means we can evolve each API and have confidence that the new version only impacts a small part of the plugin that calls this one API. So maybe a plethora of API versions for internal APIs gives teams the most velocity?

@jloleysens
Copy link
Contributor Author

jloleysens commented Mar 9, 2023

So maybe a plethora of API versions for internal APIs gives teams the most velocity?

Yeah, that is my expectation too. But I was thinking a plethora of versions per group, not a plethora of groups. So internal dashboard APIs would go 1,2,3...999...🌝.

I think managing a large set of very granular numbers would hurt velocity if we do not also provide tooling/DX to ensure you are getting the behaviour/responses you expect.

Imagine dashboard wants to introduce a breaking change to some internal endpoint. If we wanted to avoid version "explosion" for internal APIs I was envisioning teams might do something like:

// common, 1 => 2
const CURRENT_INTERNAL_VERSION=2;
// latest.ts
export * as v2 from './dashboard-types';

// server route 1 with a breaking change
router.versioned.post(...)
  .addVersion({ version: '1' }, async (ctx, req, res) => {...})
  .addVersion({ version: String(CURRENT_INTERNAL_VERSION) }, async (ctx, req, res) => {...});
 
// server route 2 that did not change
router.versioned.post(...)
  // Let's imagine we could specify a range for this handler to cover
  .addVersion({ version: `1-${String(CURRENT_INTERNAL_VERSION)} }, async (ctx, req, res) => {...});
 
// public
const internalHttpClient = http.createClient({ version: String(CURRENT_INTERNAL_VERSION) });

@jloleysens
Copy link
Contributor Author

Closing for now in favour of just exposing a versioned prop on the client: #153858

@jloleysens jloleysens closed this Mar 28, 2023
@jloleysens jloleysens deleted the versioned-http-browser-client branch March 28, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic:VersionedAPIs Kibana Versioned APIs Feature:http release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants