-
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
Latency percentile labels and instances table support #91758
Conversation
* Add "(avg.)" to dependencies table column label. (This one is always average.) * Add latency aggregation type support to the instances table. * Make the memory usage column a bit wider (it was cut off.)
Pinging @elastic/apm-ui (Team:apm) |
@@ -397,12 +405,15 @@ export const serviceInstancesRoute = createRoute({ | |||
const setup = await setupRequest(context, request); | |||
const { serviceName } = context.params.path; | |||
const { transactionType, numBuckets } = context.params.query; | |||
const latencyAggregationType = (context.params.query | |||
.latencyAggregationType as unknown) as LatencyAggregationType; |
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.
Why do you need as unknown
here?
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.
Not sure. I get this:
const latencyAggregationType: LatencyAggregationType
Type '"avg" | "p95" | "p99"' is not assignable to type 'LatencyAggregationType'.
Type '"avg"' is not assignable to type 'LatencyAggregationType'.ts(2322)
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.
I think as LatencyAggregationType
is enough, isn't it?
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 what I get when I use as LatencyAggregationType
.
@@ -96,7 +96,7 @@ export function ServiceOverviewDependenciesTable({ serviceName }: Props) { | |||
name: i18n.translate( | |||
'xpack.apm.serviceOverview.dependenciesTableColumnLatency', | |||
{ | |||
defaultMessage: 'Latency', | |||
defaultMessage: 'Latency (avg.)', |
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.
Can't we also use getLatencyColumnLabel
here?
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.
We could do getLatencyColumnLabel(undefined)
but it's always going to be avg because that's the only thing we can get with the dependencies query.
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.
LGTM just some nits
retest |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* Add "(avg.)" to dependencies table column label. (This one is always average.) * Add latency aggregation type support to the instances table. * Make the memory usage column a bit wider (it was cut off.)
* Add "(avg.)" to dependencies table column label. (This one is always average.) * Add latency aggregation type support to the instances table. * Make the memory usage column a bit wider (it was cut off.) Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
Fixes #90002. Fixes #89533.