Fix agent status lookup#25557
Conversation
💔 Build Failed |
There was a problem hiding this comment.
Does EmptyMessage still make sense to have, or should we just use EuiEmptyPrompt directly?
There was a problem hiding this comment.
This probably makes sense, punting on it for now though since I've already gone way overboard overloading this bugfix ticket lol
jasonrhodes
force-pushed
the
fix-agent-status-lookup
branch
from
c65fc6b to
029f0ed
Compare
💔 Build Failed |
|
retest |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
jasonrhodes
force-pushed
the
fix-agent-status-lookup
branch
from
1e6a333 to
6e219a2
Compare
💔 Build Failed |
|
I have a few minor comments. Is this ready for review or should I hold off a bit? |
It's ready, I think. |
|
|
||
| const ID = 'serviceList'; | ||
| const INITIAL_DATA = []; | ||
| const INITIAL_DATA: [] = []; |
There was a problem hiding this comment.
Shouldn't this be typed as IServiceListItem[]?
There was a problem hiding this comment.
Hm ... I guess? It's weird to me to type something I'm hard-coding the value of right there, esp since this value is always replaced/never mutated. I was surprised TS couldn't just infer the type?
There was a problem hiding this comment.
Thanks for fixing the casing issues here.
I think we can make the API endpoints more consistent, by having some sort of naming convention. What do you think about adding APIResponse prefix to all API response interfaces, and making them "exact", so instead of exporting a list item we export this:
export type IServiceListAPIResponse = IServiceListItem[];This way we're explicit about what the endpoint returns, and we can make it a convention to specify an *APIResponse type/interface for rest/apm.ts methods. We can even make callApi generic and require the argument like
const hits = await callApi<SpanListAPIResponse>(...)There was a problem hiding this comment.
Btw. doesn't have to be part of this PR. Just something I thought about when seeing this.
There was a problem hiding this comment.
I like this direction, makes sense to me.
There was a problem hiding this comment.
This is tightly coupled to services/get_services.ts so I think it should live in that file.
This is different from Transaction and Span that are not coupled to any endpoints.
As an example, we have a different interface for service here:
https://github.com/sqren/kibana/blob/2259b6f10eaf4d6fcbe8ab48e088df3962dae199/x-pack/plugins/apm/server/lib/services/get_service.ts#L79-L81
There was a problem hiding this comment.
My understanding of "why does something go in top-level typings" was if it's shared between server and client, which seems to make sense to me. Thoughts?
There was a problem hiding this comment.
A follow up question I would have is: do we want to have many different "service" types or should we try to align them, perhaps along with some kind of Partial implementation?
There was a problem hiding this comment.
My understanding of "why does something go in top-level typings" was if it's shared between server and client, which seems to make sense to me. Thoughts?
I think interfaces should live as close to the source as possible. I don't think it changes anything that the interface is consumed by the client, more to the contrary. Being able to link the client to the backend can be a huge advantage.
The rest methods links the frontend and the backend in runtime but it's super hard as a dev to figure this link out statically. Eg. what backend method is this frontend method calling?
https://github.com/sqren/kibana/blob/52036194b4ced069d43f1d2d924a64a9755159a2/x-pack/plugins/apm/public/services/rest/ml.js#L11
With a type defined at source in the backend, it becomes super easy to find the backend method (just click "go to definition" for ServiceResponse in your IDE):
https://github.com/sqren/kibana/blob/52036194b4ced069d43f1d2d924a64a9755159a2/x-pack/plugins/apm/public/services/rest/apm.ts#L72-L86
There was a problem hiding this comment.
do we want to have many different "service" types or should we try to align them, perhaps along with some kind of Partial implementation?
If aligning makes sense, I think we should do it. But here I'm mostly thinking about being consistent with naming and types for the same props. I don't think we should compute aggregations or send unnecessary data to the client, just to align two endpoints.
Case in point: the service endpoints
// List of services
interface ServiceListItem {
service_name: string;
agent_name: string | undefined;
transactions_per_minute: number;
errors_per_minute: number;
avg_response_time: number;
}
// Single service
interface ServiceAPIResponse {
service_name: string;
agent_name?: string;
types: string[];
}I like your idea of composing interfaces with Partial, and we could do something like:
interface Service {
service_name: string;
agent_name?: string;
}
// List of services
interface ServiceListItem extends Service {
transactions_per_minute: number;
errors_per_minute: number;
avg_response_time: number;
}
// Single service
interface ServiceAPIResponse extends Service {
types: string[];
}It introduces a level of abstraction, and reduces readability a bit but I'm ok with that.
There was a problem hiding this comment.
I like all of this in theory, but I'm not sure I understand the point of the typings folder as-is. I think in my envisioned perfect world, we would have a type for each of the main concepts in our system (Transaction, Span, Service, Trace, etc), with the variations just being pick/omit/extend versions of each but appearing in a single file for readability. Then the API response interfaces might be defined in the files that need them, and compose together the "master types" as necessary.
// typings/Service.ts
interface ApmService {
name: string;
agentName?: string;
transactionsPerMinute?: number;
errorsPerMinute?: number;
avgResponseTime?: number;
types?: string[];
}
// possibly define list vs single versions here also
// get_services.ts
export type ServiceListResponse = ApmService[] // perhaps with Omit<>
// get_service.ts
export type ServiceResponse = ApmService // perhaps with Omit<>
My question about alignment was me wondering why the list and the single return different keys for the services and if that will ever get us into trouble with assuming we have a "service" object but it's one way in the list and another way in the single entry (especially with the single entry containing less keys than the list item). If they were aligned, we wouldn't need the Omit<> at all (but maybe that's not necessary).
I don't think we should send unnecessary data to the client, just to align two endpoints.
Makes sense, but I also think it makes sense to just always return a "service" when the client asks for a service (with the one exception being maybe a list endpoint only sends a smaller "summary" rather than including the entire resource for each item in what could be a long list), so I'm not sure where I land on that.
For now, since these aren't aligned, I'll move this back to the file and rename it. We can continue to bike-shed on the typescript stuff in another ticket. :)
There was a problem hiding this comment.
I'm not sure I understand the point of the typings folder as-is
I agree. Right now it's a mess. We should look into that.
I think in my envisioned perfect world, we would have a type for each of the main concepts in our system (Transaction, Span, Service, Trace, etc), with the variations just being pick/omit/extend versions of each but appearing in a single file for readability. Then the API response interfaces might be defined in the files that need them, and compose together the "master types" as necessary.
I want this too! :) If we can make it work with the constraints (that API endpoints are not always completely aligned).
If they were aligned, we wouldn't need the Omit<> at all (but maybe that's not necessary).
We could make them almost aligned if we added transactions_per_minute, errors_per_minute and avg_response_time to the get_service endpoint. That would require us do do the following extra aggs:
{
aggs: {
avg: {
avg: { field: TRANSACTION_DURATION }
},
events: {
terms: { field: PROCESSOR_EVENT, size: 2 }
}
}
};It's not a lot but it feels a little weird to add more code, so the client will fetch more data, that we are not going to show.
But if the consistency can aid in the number of mental models we have to juggle around it might be worth it.
There was a problem hiding this comment.
Yeah ... I mean it's a good point you make. I honestly could argue it either way. I'll leave it alone for now and we'll keep thinking on it!
💚 Build Succeeded |
💚 Build Succeeded |
|
@sqren I typed that initial data and moved that type back to the get_services.ts file 👍 |
💚 Build Succeeded |
* Updates service overview to only do agent status check once * TS support added for EmptyMessage and those who import it * Fixed up tests * Finishes TS and test updates for agent check fix * Fixing up tests * Fixed linting error * Updated broken tests * Re-typed the service list response, camelCase throughout * Fixes broken test after data change * Makes initial data type more specific * Moves service list item TS interface back into server lib file
* Updates service overview to only do agent status check once * TS support added for EmptyMessage and those who import it * Fixed up tests * Finishes TS and test updates for agent check fix * Fixing up tests * Fixed linting error * Updated broken tests * Re-typed the service list response, camelCase throughout * Fixes broken test after data change * Makes initial data type more specific * Moves service list item TS interface back into server lib file
3 participants
Closes #25436
Summary
This PR swaps out the agent status checks for the APM service list page and replaces them with a single check that only happens on mount. Also a few typescript file conversions.