-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[APM] migrate to io-ts #42961
[APM] migrate to io-ts #42961
Conversation
|
||
export type APICall<T extends ServerAPI<{}>> = T['call']; | ||
|
||
const api: ServerAPI<{}> = {} as any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this and below is just dummy stuff to test out the types.
💔 Build Failed |
@sqren We can also use this approach to generate the methods we now have in {
name: 'getErrorDistribution',
path: `/api/apm/services/{serviceName}/errors/distribution`,
method: 'GET',
params: {
path: t.type({
serviceName: t.string
}),
query: t.intersection([
t.partial({
groupId: t.string
}),
defaultApiTypes
])
},
handler: async (req, params) => {
...
}
} to const { client } = api;
client.getErrorDistribution({ serviceName, groupId, start, end, uiFilters }) And it would be fully typed. Thoughts? |
x-pack/legacy/plugins/apm/public/services/rest/apm/error_groups.ts
Outdated
Show resolved
Hide resolved
My knee-jerk reaction was that I did not like this. Where's the http method, the url, potentially headers if we want to add those? Controversial idea: we could take a page from graphql's book and get rid of path, method and headers entirely (all requests go to a single endpoint). We could keep Btw. I am too young to remember xml-rpc but I believe that is what we are reinventing. I love it! :D |
@sqren maybe we should use |
I also don't think it would be a great idea. For one it becomes much harder to cache content if everything goes to the same endpoint.
The benefit would be that it would simplify the API. Right now a route is identified by both a With the rpc style approach you suggested last (which I liked) we don't have a single identifier. Eg. from the client we call it like client.getErrorDistribution({ serviceName, groupId, start, end, uiFilters }) So the unique identifier for the route is |
I guess that's up to the implementation. Nothing is holding us back from using |
Apart from the odd camelCased paths we'd end up with I think I like this approach. What do you think? |
Yeah, I like it. Maybe I can demo what I have in today's standup, I'd like to know what @ogupte thinks about it as well. |
Thinking about this some more: can we import server code (specifically the route descriptions) for client code? I mean runtime code, not just types. If we can't, we have to consider that the client call that we make needs to know enough to construct a request object. Ie, it needs to know the pathname, the request method, and the parameters. So that would limit us to the following API I think: [RouteName]: ( params:Params, method:HttpMethod = 'GET' ) => ReturnTypeOfRoute So that means even if we only have a POST route for RouteName, we would still need to explicitly define 'POST' as a method, because the client needs to know that it should send a POST to that API. (this also means that we still have to differentiate between |
It does seems like I'm not able to import server-side code. The errors are really vague, but I'm going to assume that it just doesn't work for the time being. That also means I have to implement the pattern that we discussed export const calls: Client<APMAPI> = new Proxy(
{},
{
get: (obj, prop) => {
return ({
query = {},
body = {},
method = 'GET'
}) => {
return callApi({
method,
pathname: `/api/apm/${prop.toString()}`,
query,
body: body ? JSON.stringify(body) : undefined
}) as any;
};
}
}
) as Client<APMAPI>; We can also keep it simple and opt for |
You might already do this but just to make sure we are on the same page: Typescript imports (interfaces, types etc) should work across |
💔 Build Failed |
💔 Build Failed |
x-pack/legacy/plugins/apm/public/components/app/ErrorGroupDetails/index.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed |
t.number.is, | ||
(input, context) => { | ||
const value = Number(input); | ||
return value >= 0 && value <= 1 && Number(value.toFixed(3)) === value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number(value.toFixed(3)) === value
looks so much nicer than the one I suggested 👍
// add _debug query parameter to all routes | ||
if (key === 'query') { | ||
codec = t.intersection([codec, debugRt]); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used to do:
params: {
query: params.query
? t.intersection([params.query, debugRt])
: debugRt
},
Which made sense to me. But with this change, if the route doesn't define a query
will _debug still be added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's a good catch actually. The way it's written now none of the {query,path,body} parameters get validated if there's no param types. Let me fix that (and write a test for it). Thanks!
💚 Build Succeeded |
💔 Build Failed |
retest |
💚 Build Succeeded |
* [APM] migrate to io-ts * Migrate remaining routes to io-ts * Infer response type for useFetcher() * Review feedback * Use createRangeType util * Extract & test runtime types * Simplify runtime types * Tests for createApi and callApmApi * Use more readable variable names in runtime types * Remove UIFilters query param for API endpoints where it is not supported * Fix issues w/ default parameters in create_api
* [APM] migrate to io-ts * Migrate remaining routes to io-ts * Infer response type for useFetcher() * Review feedback * Use createRangeType util * Extract & test runtime types * Simplify runtime types * Tests for createApi and callApmApi * Use more readable variable names in runtime types * Remove UIFilters query param for API endpoints where it is not supported * Fix issues w/ default parameters in create_api
I've migrated some of our routes to io-ts, and with the help of some TypeScript magic and a quirky way of registering our routes, we now should have end-to-end runtime and type coverage without having to duplicate things for client and server.
Opening a draft PR to get some early feedback. I migrated the error and index pattern routes.
To summarize:
router
vessel. that router will inherit the types of the routes, and merge them all together.createRoute
call. this allows us to get fully typed parameters in the request handler, inferred via theio-ts
types.router
vessel is used to create a fully typedcallApmApi
method, that usespathname
andmethod
as indexers to get inferred types for path, query and body params, and return types.