-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Optimize traces overview #70200
Changes from 7 commits
2b1f4c3
e7ac578
bfac5c2
43bec06
fcf18b2
fd73424
f80ff38
0395763
dff5fdc
5126db9
804df26
6bf8dbb
0bee339
381304f
a13cfc6
9fb2d12
ccf9654
b040a40
861cf63
72eb667
a86551b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
import { ValuesType } from 'utility-types'; | ||
|
||
// work around a TypeScript limitation described in https://stackoverflow.com/posts/49511416 | ||
|
||
export const arrayUnionToCallable = <T extends any[] | undefined>( | ||
array: T | ||
): Array<ValuesType<Exclude<T, undefined>>> => { | ||
return array ?? []; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,45 +4,15 @@ | |
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useMemo } from 'react'; | ||
import { IUrlParams } from '../context/UrlParamsContext/types'; | ||
import { useUiFilters } from '../context/UrlParamsContext'; | ||
import { useFetcher } from './useFetcher'; | ||
import { APIReturnType } from '../services/rest/createCallApmApi'; | ||
|
||
const getRelativeImpact = ( | ||
impact: number, | ||
impactMin: number, | ||
impactMax: number | ||
) => | ||
Math.max( | ||
((impact - impactMin) / Math.max(impactMax - impactMin, 1)) * 100, | ||
1 | ||
); | ||
|
||
type TransactionsAPIResponse = APIReturnType< | ||
'/api/apm/services/{serviceName}/transaction_groups' | ||
>; | ||
|
||
function getWithRelativeImpact(items: TransactionsAPIResponse['items']) { | ||
const impacts = items | ||
.map(({ impact }) => impact) | ||
.filter((impact) => impact !== null) as number[]; | ||
|
||
const impactMin = Math.min(...impacts); | ||
const impactMax = Math.max(...impacts); | ||
|
||
return items.map((item) => { | ||
return { | ||
...item, | ||
impactRelative: | ||
item.impact !== null | ||
? getRelativeImpact(item.impact, impactMin, impactMax) | ||
: null, | ||
}; | ||
}); | ||
} | ||
|
||
const DEFAULT_RESPONSE: TransactionsAPIResponse = { | ||
items: [], | ||
isAggregationAccurate: true, | ||
|
@@ -72,16 +42,8 @@ export function useTransactionList(urlParams: IUrlParams) { | |
[serviceName, start, end, transactionType, uiFilters] | ||
); | ||
|
||
const memoizedData = useMemo( | ||
() => ({ | ||
items: getWithRelativeImpact(data.items), | ||
isAggregationAccurate: data.isAggregationAccurate, | ||
bucketSize: data.bucketSize, | ||
}), | ||
[data] | ||
); | ||
return { | ||
data: memoizedData, | ||
data, | ||
status, | ||
error, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for cleaning up 👍 |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,7 +157,7 @@ export function registerTransactionDurationAlertType({ | |
|
||
const { agg } = response.aggregations; | ||
|
||
const value = 'values' in agg ? agg.values[0] : agg?.value; | ||
const value = 'values' in agg ? Object.values(agg.values)[0] : agg?.value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this an unrelated change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, this is a bug fix now that I think of it. This code (written by me of course) assumes that the response of a percentiles aggregation is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
if (value && value > alertParams.threshold * 1000) { | ||
const alertInstance = services.alertInstanceFactory( | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,7 +4,8 @@ | |||||
* you may not use this file except in compliance with the Elastic License. | ||||||
*/ | ||||||
|
||||||
import { Unionize } from 'utility-types'; | ||||||
import { Unionize, Overwrite } from 'utility-types'; | ||||||
import { ESSearchRequest } from '../../../typings/elasticsearch'; | ||||||
import { | ||||||
Setup, | ||||||
SetupTimeRange, | ||||||
|
@@ -17,14 +18,28 @@ import { getMetricsProjection } from '../../../common/projections/metrics'; | |||||
import { mergeProjection } from '../../../common/projections/util/merge_projection'; | ||||||
import { AggregationOptionsByType } from '../../../typings/elasticsearch/aggregations'; | ||||||
|
||||||
interface Aggs { | ||||||
[key: string]: Unionize<{ | ||||||
min: AggregationOptionsByType['min']; | ||||||
max: AggregationOptionsByType['max']; | ||||||
sum: AggregationOptionsByType['sum']; | ||||||
avg: AggregationOptionsByType['avg']; | ||||||
}>; | ||||||
} | ||||||
type MetricsAggregationMap = Unionize<{ | ||||||
min: AggregationOptionsByType['min']; | ||||||
max: AggregationOptionsByType['max']; | ||||||
sum: AggregationOptionsByType['sum']; | ||||||
avg: AggregationOptionsByType['avg']; | ||||||
}>; | ||||||
|
||||||
type Aggs = Record<string, MetricsAggregationMap>; | ||||||
|
||||||
export type GenericMetricsRequest = Overwrite< | ||||||
ESSearchRequest, | ||||||
{ | ||||||
body: { | ||||||
aggs: { | ||||||
timeseriesData: { | ||||||
date_histogram: AggregationOptionsByType['date_histogram']; | ||||||
aggs: Record<string, MetricsAggregationMap>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this just be:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep! Changed it to |
||||||
}; | ||||||
} & Record<string, MetricsAggregationMap>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this just be:
Suggested change
|
||||||
}; | ||||||
} | ||||||
>; | ||||||
|
||||||
interface Filter { | ||||||
exists?: { | ||||||
|
@@ -58,7 +73,7 @@ export async function fetchAndTransformMetrics<T extends Aggs>({ | |||||
serviceNodeName, | ||||||
}); | ||||||
|
||||||
const params = mergeProjection(projection, { | ||||||
const params: GenericMetricsRequest = mergeProjection(projection, { | ||||||
body: { | ||||||
size: 0, | ||||||
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.
I know this line was just name changes, but still: I don't think the id
trace-transaction-link-tooltip
is necessary anymore. I don't see it referenced anywhere, and i don't think it's used in any unit/functional testing.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.
Removed