From 5f5d4452c9e7050b6fbc978e065c679e316e8c70 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 8 Aug 2022 14:42:55 +0200 Subject: [PATCH 01/11] Fix range steps for latency correlations chart to be in sync. --- ...use_transaction_distribution_chart_data.ts | 8 +++- .../fetch_duration_histogram_range_steps.ts | 39 +++++++++++++++---- .../get_overall_latency_distribution.ts | 30 ++++++++------ .../routes/latency_distribution/route.ts | 6 +++ .../routes/latency_distribution/types.ts | 2 + 5 files changed, 65 insertions(+), 20 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/use_transaction_distribution_chart_data.ts b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/use_transaction_distribution_chart_data.ts index 42febda7a186f..77f284e0ffee7 100644 --- a/x-pack/plugins/apm/public/components/app/transaction_details/distribution/use_transaction_distribution_chart_data.ts +++ b/x-pack/plugins/apm/public/components/app/transaction_details/distribution/use_transaction_distribution_chart_data.ts @@ -86,7 +86,9 @@ export const useTransactionDistributionChartData = () => { params.serviceName && params.environment && params.start && - params.end + params.end && + overallLatencyData.durationMin && + overallLatencyData.durationMax ) { return callApmApi( 'POST /internal/apm/latency/overall_distribution/transactions', @@ -94,6 +96,8 @@ export const useTransactionDistributionChartData = () => { params: { body: { ...params, + durationMin: overallLatencyData.durationMin, + durationMax: overallLatencyData.durationMax, percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, termFilters: [ { @@ -108,7 +112,7 @@ export const useTransactionDistributionChartData = () => { ); } }, - [params] + [params, overallLatencyData.durationMin, overallLatencyData.durationMax] ); useEffect(() => { diff --git a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_histogram_range_steps.ts b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_histogram_range_steps.ts index 913bec60d9de8..f1ba0347f7fcc 100644 --- a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_histogram_range_steps.ts +++ b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_duration_histogram_range_steps.ts @@ -32,14 +32,35 @@ export const fetchDurationHistogramRangeSteps = async ({ kuery, query, searchMetrics, + durationMinOverride, + durationMaxOverride, }: CommonCorrelationsQueryParams & { chartType: LatencyDistributionChartType; setup: Setup; searchMetrics: boolean; -}): Promise => { + durationMinOverride?: number; + durationMaxOverride?: number; +}): Promise<{ + durationMin?: number; + durationMax?: number; + rangeSteps: number[]; +}> => { + const steps = 100; + + if (durationMinOverride && durationMaxOverride) { + return { + durationMin: durationMinOverride, + durationMax: durationMaxOverride, + rangeSteps: getHistogramRangeSteps( + durationMinOverride, + durationMaxOverride, + steps + ), + }; + } + const { apmEventClient } = setup; - const steps = 100; const durationField = getDurationField(chartType, searchMetrics); // when using metrics data, ensure we filter by docs with the appropriate duration field @@ -71,7 +92,7 @@ export const fetchDurationHistogramRangeSteps = async ({ ); if (resp.hits.total.value === 0) { - return getHistogramRangeSteps(0, 1, 100); + return { rangeSteps: getHistogramRangeSteps(0, 1, 100) }; } if ( @@ -81,11 +102,15 @@ export const fetchDurationHistogramRangeSteps = async ({ isFiniteNumber(resp.aggregations.duration_max.value) ) ) { - return []; + return { rangeSteps: [] }; } - const min = resp.aggregations.duration_min.value; - const max = resp.aggregations.duration_max.value * 2; + const durationMin = resp.aggregations.duration_min.value; + const durationMax = resp.aggregations.duration_max.value * 2; - return getHistogramRangeSteps(min, max, steps); + return { + durationMin, + durationMax, + rangeSteps: getHistogramRangeSteps(durationMin, durationMax, steps), + }; }; diff --git a/x-pack/plugins/apm/server/routes/latency_distribution/get_overall_latency_distribution.ts b/x-pack/plugins/apm/server/routes/latency_distribution/get_overall_latency_distribution.ts index b44c071f2ab37..206deb8a32d15 100644 --- a/x-pack/plugins/apm/server/routes/latency_distribution/get_overall_latency_distribution.ts +++ b/x-pack/plugins/apm/server/routes/latency_distribution/get_overall_latency_distribution.ts @@ -28,6 +28,8 @@ export async function getOverallLatencyDistribution({ kuery, query, percentileThreshold, + durationMinOverride, + durationMaxOverride, searchMetrics, }: { chartType: LatencyDistributionChartType; @@ -38,6 +40,8 @@ export async function getOverallLatencyDistribution({ kuery: string; query: estypes.QueryDslQueryContainer; percentileThreshold: number; + durationMinOverride?: number; + durationMaxOverride?: number; searchMetrics: boolean; }) { return withApmSpan('get_overall_latency_distribution', async () => { @@ -63,23 +67,25 @@ export async function getOverallLatencyDistribution({ } // #2: get histogram range steps - const rangeSteps = await fetchDurationHistogramRangeSteps({ - chartType, - setup, - start, - end, - environment, - kuery, - query, - searchMetrics, - }); + const { durationMin, durationMax, rangeSteps } = + await fetchDurationHistogramRangeSteps({ + chartType, + setup, + start, + end, + environment, + kuery, + query, + searchMetrics, + durationMinOverride, + durationMaxOverride, + }); if (!rangeSteps) { return overallLatencyDistribution; } // #3: get histogram chart data - const { totalDocCount, durationRanges } = await fetchDurationRanges({ chartType, setup, @@ -92,6 +98,8 @@ export async function getOverallLatencyDistribution({ searchMetrics, }); + overallLatencyDistribution.durationMin = durationMin; + overallLatencyDistribution.durationMax = durationMax; overallLatencyDistribution.totalDocCount = totalDocCount; overallLatencyDistribution.overallHistogram = durationRanges; diff --git a/x-pack/plugins/apm/server/routes/latency_distribution/route.ts b/x-pack/plugins/apm/server/routes/latency_distribution/route.ts index 5fc2072bad224..7eaaa6e641030 100644 --- a/x-pack/plugins/apm/server/routes/latency_distribution/route.ts +++ b/x-pack/plugins/apm/server/routes/latency_distribution/route.ts @@ -38,6 +38,8 @@ const latencyOverallTransactionDistributionRoute = createApmServerRoute({ fieldValue: t.union([t.string, toNumberRt]), }) ), + durationMin: toNumberRt, + durationMax: toNumberRt, }), environmentRt, kueryRt, @@ -63,6 +65,8 @@ const latencyOverallTransactionDistributionRoute = createApmServerRoute({ start, end, percentileThreshold, + durationMin, + durationMax, termFilters, chartType, } = resources.params.body; @@ -99,6 +103,8 @@ const latencyOverallTransactionDistributionRoute = createApmServerRoute({ }, }, percentileThreshold, + durationMinOverride: durationMin, + durationMaxOverride: durationMax, searchMetrics: searchAggregatedTransactions, }); }, diff --git a/x-pack/plugins/apm/server/routes/latency_distribution/types.ts b/x-pack/plugins/apm/server/routes/latency_distribution/types.ts index 92129f1a5f234..4fb19eba4e57d 100644 --- a/x-pack/plugins/apm/server/routes/latency_distribution/types.ts +++ b/x-pack/plugins/apm/server/routes/latency_distribution/types.ts @@ -18,6 +18,8 @@ export interface OverallLatencyDistributionOptions { } export interface OverallLatencyDistributionResponse { + durationMin?: number; + durationMax?: number; totalDocCount?: number; percentileThresholdValue?: number | null; overallHistogram?: Array<{ From f8c3ef636db61a7e51e66da8efafcb0deca3d1c7 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 8 Aug 2022 14:44:37 +0200 Subject: [PATCH 02/11] Fix duration overrides for latency correlations and faild transaction correlations. --- .../use_failed_transactions_correlations.ts | 93 +++++++++++-------- .../correlations/use_latency_correlations.ts | 38 +++++--- .../correlations/queries/fetch_p_values.ts | 8 +- .../queries/fetch_significant_correlations.ts | 12 ++- .../apm/server/routes/correlations/route.ts | 12 +++ 5 files changed, 104 insertions(+), 59 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts index 4119d9e2e4dae..85246c07f5bb7 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts +++ b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts @@ -85,48 +85,54 @@ export function useFailedTransactionsCorrelations() { fallbackResult: undefined, }; - const [overallHistogramResponse, errorHistogramRespone] = - await Promise.all([ - // Initial call to fetch the overall distribution for the log-log plot. - callApmApi( - 'POST /internal/apm/latency/overall_distribution/transactions', - { - signal: abortCtrl.current.signal, - params: { - body: { - ...fetchParams, - percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, - chartType: - LatencyDistributionChartType.failedTransactionsCorrelations, - }, - }, - } - ), - callApmApi( - 'POST /internal/apm/latency/overall_distribution/transactions', - { - signal: abortCtrl.current.signal, - params: { - body: { - ...fetchParams, - percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, - termFilters: [ - { - fieldName: EVENT_OUTCOME, - fieldValue: EventOutcome.failure, - }, - ], - chartType: - LatencyDistributionChartType.failedTransactionsCorrelations, + // Initial call to fetch the overall distribution for the log-log plot. + const overallHistogramResponse = await callApmApi( + 'POST /internal/apm/latency/overall_distribution/transactions', + { + signal: abortCtrl.current.signal, + params: { + body: { + ...fetchParams, + percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, + chartType: + LatencyDistributionChartType.failedTransactionsCorrelations, + }, + }, + } + ); + + const { + overallHistogram, + totalDocCount, + percentileThresholdValue, + durationMin, + durationMax, + } = overallHistogramResponse; + + const errorHistogramResponse = await callApmApi( + 'POST /internal/apm/latency/overall_distribution/transactions', + { + signal: abortCtrl.current.signal, + params: { + body: { + ...fetchParams, + percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, + termFilters: [ + { + fieldName: EVENT_OUTCOME, + fieldValue: EventOutcome.failure, }, - }, - } - ), - ]); + ], + durationMin, + durationMax, + chartType: + LatencyDistributionChartType.failedTransactionsCorrelations, + }, + }, + } + ); - const { overallHistogram, totalDocCount, percentileThresholdValue } = - overallHistogramResponse; - const { overallHistogram: errorHistogram } = errorHistogramRespone; + const { overallHistogram: errorHistogram } = errorHistogramResponse; responseUpdate.errorHistogram = errorHistogram; responseUpdate.overallHistogram = overallHistogram; @@ -179,7 +185,12 @@ export function useFailedTransactionsCorrelations() { { signal: abortCtrl.current.signal, params: { - body: { ...fetchParams, fieldCandidates: fieldCandidatesChunk }, + body: { + ...fetchParams, + fieldCandidates: fieldCandidatesChunk, + durationMin, + durationMax, + }, }, } ); diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts b/x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts index b46aaf815d6ad..b2ba6fc4c68b1 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts +++ b/x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts @@ -86,20 +86,25 @@ export function useLatencyCorrelations() { }; // Initial call to fetch the overall distribution for the log-log plot. - const { overallHistogram, totalDocCount, percentileThresholdValue } = - await callApmApi( - 'POST /internal/apm/latency/overall_distribution/transactions', - { - signal: abortCtrl.current.signal, - params: { - body: { - ...fetchParams, - percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, - chartType: LatencyDistributionChartType.latencyCorrelations, - }, + const { + overallHistogram, + totalDocCount, + percentileThresholdValue, + durationMin, + durationMax, + } = await callApmApi( + 'POST /internal/apm/latency/overall_distribution/transactions', + { + signal: abortCtrl.current.signal, + params: { + body: { + ...fetchParams, + percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD, + chartType: LatencyDistributionChartType.latencyCorrelations, }, - } - ); + }, + } + ); responseUpdate.overallHistogram = overallHistogram; responseUpdate.totalDocCount = totalDocCount; responseUpdate.percentileThresholdValue = percentileThresholdValue; @@ -192,7 +197,12 @@ export function useLatencyCorrelations() { { signal: abortCtrl.current.signal, params: { - body: { ...fetchParams, fieldValuePairs: fieldValuePairChunk }, + body: { + ...fetchParams, + durationMin, + durationMax, + fieldValuePairs: fieldValuePairChunk, + }, }, } ); diff --git a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_p_values.ts b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_p_values.ts index f52c9bcfa51c6..72cd6baaefec4 100644 --- a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_p_values.ts +++ b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_p_values.ts @@ -22,16 +22,20 @@ export const fetchPValues = async ({ environment, kuery, query, + durationMin, + durationMax, fieldCandidates, }: CommonCorrelationsQueryParams & { setup: Setup; + durationMin?: number; + durationMax?: number; fieldCandidates: string[]; }) => { const chartType = LatencyDistributionChartType.failedTransactionsCorrelations; const searchMetrics = false; // failed transactions correlations does not search metrics documents const eventType = getEventType(chartType, searchMetrics); - const rangeSteps = await fetchDurationHistogramRangeSteps({ + const { rangeSteps } = await fetchDurationHistogramRangeSteps({ setup, chartType, start, @@ -40,6 +44,8 @@ export const fetchPValues = async ({ kuery, query, searchMetrics, + durationMinOverride: durationMin, + durationMaxOverride: durationMax, }); const { fulfilled, rejected } = splitAllSettledPromises( diff --git a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_significant_correlations.ts b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_significant_correlations.ts index 9799f75d82241..abb23fbfac0e8 100644 --- a/x-pack/plugins/apm/server/routes/correlations/queries/fetch_significant_correlations.ts +++ b/x-pack/plugins/apm/server/routes/correlations/queries/fetch_significant_correlations.ts @@ -34,9 +34,13 @@ export const fetchSignificantCorrelations = async ({ environment, kuery, query, + durationMinOverride, + durationMaxOverride, fieldValuePairs, }: CommonCorrelationsQueryParams & { setup: Setup; + durationMinOverride?: number; + durationMaxOverride?: number; fieldValuePairs: FieldValuePair[]; }) => { // Create an array of ranges [2, 4, 6, ..., 98] @@ -75,7 +79,7 @@ export const fetchSignificantCorrelations = async ({ ranges, }); - const histogramRangeSteps = await fetchDurationHistogramRangeSteps({ + const { rangeSteps } = await fetchDurationHistogramRangeSteps({ setup, chartType, start, @@ -84,6 +88,8 @@ export const fetchSignificantCorrelations = async ({ kuery, query, searchMetrics, + durationMinOverride, + durationMaxOverride, }); const { fulfilled, rejected } = splitAllSettledPromises( @@ -100,7 +106,7 @@ export const fetchSignificantCorrelations = async ({ expectations, ranges, fractions, - histogramRangeSteps, + histogramRangeSteps: rangeSteps, totalDocCount, fieldValuePair, }) @@ -147,7 +153,7 @@ export const fetchSignificantCorrelations = async ({ filter: [query, ...termQuery(fieldName, fieldValue)], }, }, - rangeSteps: histogramRangeSteps, + rangeSteps, searchMetrics, }); diff --git a/x-pack/plugins/apm/server/routes/correlations/route.ts b/x-pack/plugins/apm/server/routes/correlations/route.ts index 4000dcb53676e..fd3a405bc7a95 100644 --- a/x-pack/plugins/apm/server/routes/correlations/route.ts +++ b/x-pack/plugins/apm/server/routes/correlations/route.ts @@ -307,6 +307,8 @@ const significantCorrelationsTransactionsRoute = createApmServerRoute({ serviceName: t.string, transactionName: t.string, transactionType: t.string, + durationMin: toNumberRt, + durationMax: toNumberRt, }), environmentRt, kueryRt, @@ -343,6 +345,8 @@ const significantCorrelationsTransactionsRoute = createApmServerRoute({ end, environment, kuery, + durationMin, + durationMax, fieldValuePairs, }, } = resources.params; @@ -362,6 +366,8 @@ const significantCorrelationsTransactionsRoute = createApmServerRoute({ ], }, }, + durationMinOverride: durationMin, + durationMaxOverride: durationMax, fieldValuePairs, }); }, @@ -375,6 +381,8 @@ const pValuesTransactionsRoute = createApmServerRoute({ serviceName: t.string, transactionName: t.string, transactionType: t.string, + durationMin: toNumberRt, + durationMax: toNumberRt, }), environmentRt, kueryRt, @@ -405,6 +413,8 @@ const pValuesTransactionsRoute = createApmServerRoute({ end, environment, kuery, + durationMin, + durationMax, fieldCandidates, }, } = resources.params; @@ -424,6 +434,8 @@ const pValuesTransactionsRoute = createApmServerRoute({ ], }, }, + durationMin, + durationMax, fieldCandidates, }); }, From b0c2e0df3ce2aeac84c73173f7d11acf3741807e Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 8 Aug 2022 16:57:23 +0200 Subject: [PATCH 03/11] [ML] Fix tests. --- .../correlations/use_failed_transactions_correlations.test.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx index 76eea019ac83e..2fef9799c9566 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx @@ -173,6 +173,8 @@ describe('useFailedTransactionsCorrelations', () => { jest.advanceTimersByTime(100); await waitFor(() => expect(result.current.progress.loaded).toBe(0)); jest.advanceTimersByTime(100); + await waitFor(() => expect(result.current.progress.loaded).toBe(0)); + jest.advanceTimersByTime(100); await waitFor(() => expect(result.current.progress.loaded).toBe(0.05)); expect(result.current.progress).toEqual({ From 35c5021487b31ff2023763cf2c27213180c3f70f Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 8 Aug 2022 17:56:20 +0200 Subject: [PATCH 04/11] [ML] Make use of updated Elastic Charts available styles for orphaned points. --- .../duration_distribution_chart/index.tsx | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx index 8e688832f7f36..712ba0e8952d3 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx @@ -83,18 +83,17 @@ const getAnnotationsStyle = (color = 'gray'): LineAnnotationStyle => ({ }, }); -// TODO Revisit this approach since it actually manipulates the numbers -// showing in the chart and its tooltips. -const CHART_PLACEHOLDER_VALUE = 0.0001; +// With a log axis, Elastic Charts would not draw a continuous line for values that are 0. +// By replacing the 0s with a minimum domain value of >0 the line will be drawn as intended. +// This is just to visually fix the line, for tooltips, that number will be again rounded down to 0. +const Y_AXIS_MIN_DOMAIN = 0.5; -// Elastic charts will show any lone bin (i.e. a populated bin followed by empty bin) -// as a circular marker instead of a bar -// This provides a workaround by making the next bin not empty -// TODO Find a way to get rid of this workaround since it alters original values of the data. -export const replaceHistogramDotsWithBars = (histogramItems: HistogramItem[]) => +export const replaceHistogramZeroesWithMinimumDomainValue = ( + histogramItems: HistogramItem[] +) => histogramItems.reduce((histogramItem, _, i) => { if (histogramItem[i].doc_count === 0) { - histogramItem[i].doc_count = CHART_PLACEHOLDER_VALUE; + histogramItem[i].doc_count = Y_AXIS_MIN_DOMAIN; } return histogramItem; }, histogramItems); @@ -141,7 +140,7 @@ export function DurationDistributionChart({ ) ?? 0; const yTicks = Math.max(1, Math.ceil(Math.log10(yMax))); const yAxisDomain = { - min: 0.5, + min: Y_AXIS_MIN_DOMAIN, max: Math.pow(10, yTicks), }; @@ -178,6 +177,10 @@ export function DurationDistributionChart({ line: { visible: true, }, + point: { + visible: false, + radius: 0, + }, }, axes: { tickLine: { @@ -266,26 +269,28 @@ export function DurationDistributionChart({ ticks={yTicks} gridLine={{ visible: true }} /> - {data.map((d, i) => ( + {data.map((d) => ( `${Math.round(p)}`} + tickFormat={(p) => `${Math.floor(p)}`} /> ))} From 64fff2930c986fe87b12337e1e6eabd606252ec1 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Mon, 8 Aug 2022 19:53:01 +0200 Subject: [PATCH 05/11] [ML] Fix tests. --- .../duration_distribution_chart/index.test.tsx | 18 ++++++++++-------- .../duration_distribution_chart/index.tsx | 4 ++-- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx index 6af1c89fd1991..e9fa4aae02d7c 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx @@ -7,11 +7,11 @@ import type { HistogramItem } from '../../../../../common/correlations/types'; -import { replaceHistogramDotsWithBars } from '.'; +import { replaceHistogramZerosWithMinimumDomainValue } from '.'; describe('TransactionDistributionChart', () => { - describe('replaceHistogramDotsWithBars', () => { - it('does the thing', () => { + describe('replaceHistogramZerosWithMinimumDomainValue', () => { + it('replaces zeroes', () => { const mockHistogram = [ { doc_count: 10 }, { doc_count: 10 }, @@ -25,15 +25,17 @@ describe('TransactionDistributionChart', () => { { doc_count: 10 }, ] as HistogramItem[]; - expect(replaceHistogramDotsWithBars(mockHistogram)).toEqual([ + expect( + replaceHistogramZerosWithMinimumDomainValue(mockHistogram) + ).toEqual([ { doc_count: 10 }, { doc_count: 10 }, - { doc_count: 0.0001 }, - { doc_count: 0.0001 }, - { doc_count: 0.0001 }, + { doc_count: 0.5 }, + { doc_count: 0.5 }, + { doc_count: 0.5 }, { doc_count: 10 }, { doc_count: 10 }, - { doc_count: 0.0001 }, + { doc_count: 0.5 }, { doc_count: 10 }, { doc_count: 10 }, ]); diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx index 712ba0e8952d3..9fdbe77a6e7fc 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx @@ -88,7 +88,7 @@ const getAnnotationsStyle = (color = 'gray'): LineAnnotationStyle => ({ // This is just to visually fix the line, for tooltips, that number will be again rounded down to 0. const Y_AXIS_MIN_DOMAIN = 0.5; -export const replaceHistogramZeroesWithMinimumDomainValue = ( +export const replaceHistogramZerosWithMinimumDomainValue = ( histogramItems: HistogramItem[] ) => histogramItems.reduce((histogramItem, _, i) => { @@ -275,7 +275,7 @@ export function DurationDistributionChart({ id={d.id} xScaleType={ScaleType.Log} yScaleType={ScaleType.Log} - data={replaceHistogramZeroesWithMinimumDomainValue(d.histogram)} + data={replaceHistogramZerosWithMinimumDomainValue(d.histogram)} curve={CurveType.CURVE_STEP_AFTER} xAccessor="key" yAccessors={['doc_count']} From 4ad0a05e622756287a64e4f681bf9240855acbfc Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Wed, 10 Aug 2022 11:16:39 +0200 Subject: [PATCH 06/11] [ML] Add comment to clarify test structure. --- .../correlations/use_failed_transactions_correlations.test.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx index 2fef9799c9566..1464e6411d497 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx @@ -168,6 +168,9 @@ describe('useFailedTransactionsCorrelations', () => { ); try { + // Each simulated request takes 100ms. After an inital 50ms + // we track the internal requests the hook is running and + // check the expected progress after these requests. jest.advanceTimersByTime(50); await waitFor(() => expect(result.current.progress.loaded).toBe(0)); jest.advanceTimersByTime(100); From cde87ac6b3ab15b92a5a7f74f7b18c6187e80780 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 11 Aug 2022 09:43:48 +0200 Subject: [PATCH 07/11] Revert chart style to not show continuous line. --- .../charts/duration_distribution_chart/index.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx index 9fdbe77a6e7fc..cde84c4daac20 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx @@ -83,17 +83,21 @@ const getAnnotationsStyle = (color = 'gray'): LineAnnotationStyle => ({ }, }); -// With a log axis, Elastic Charts would not draw a continuous line for values that are 0. -// By replacing the 0s with a minimum domain value of >0 the line will be drawn as intended. +// With a log based y axis in combination with the `CURVE_STEP_AFTER` style, +// the line of an area would not go down to 0 but end on the y axis at the last value >0. +// By replacing the 0s with a small value >0 the line will be drawn as intended. // This is just to visually fix the line, for tooltips, that number will be again rounded down to 0. +// Note this workaround is only safe to use for this type of chart because it works with +// count based values and not a float based metric for example on the y axis. const Y_AXIS_MIN_DOMAIN = 0.5; +const Y_AXIS_MIN_VALUE = 0.0001; export const replaceHistogramZerosWithMinimumDomainValue = ( histogramItems: HistogramItem[] ) => histogramItems.reduce((histogramItem, _, i) => { if (histogramItem[i].doc_count === 0) { - histogramItem[i].doc_count = Y_AXIS_MIN_DOMAIN; + histogramItem[i].doc_count = Y_AXIS_MIN_VALUE; } return histogramItem; }, histogramItems); From 52cf77ecea2b6a95a901af721beae8c97d5bcf4c Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 11 Aug 2022 09:48:21 +0200 Subject: [PATCH 08/11] Move the data transform to a useMemo. --- .../charts/duration_distribution_chart/index.tsx | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx index cde84c4daac20..d6a82433c887b 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx @@ -5,7 +5,7 @@ * 2.0. */ -import React from 'react'; +import React, { useMemo } from 'react'; import { flatten } from 'lodash'; import { @@ -163,6 +163,15 @@ export function DurationDistributionChart({ ] : undefined; + const chartData = useMemo( + () => + data.map((d) => ({ + ...d, + histogram: replaceHistogramZerosWithMinimumDomainValue(d.histogram), + })), + [data] + ); + return (
- {data.map((d) => ( + {chartData.map((d) => ( Date: Thu, 11 Aug 2022 10:01:01 +0200 Subject: [PATCH 09/11] Set the height of the selection annotation dynamically based on the max y domain value. --- .../shared/charts/duration_distribution_chart/index.tsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx index d6a82433c887b..49885977b57dd 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.tsx @@ -143,9 +143,10 @@ export function DurationDistributionChart({ ...flatten(data.map((d) => d.histogram)).map((d) => d.doc_count) ) ?? 0; const yTicks = Math.max(1, Math.ceil(Math.log10(yMax))); + const yAxisMaxDomain = Math.pow(10, yTicks); const yAxisDomain = { min: Y_AXIS_MIN_DOMAIN, - max: Math.pow(10, yTicks), + max: yAxisMaxDomain, }; const selectionAnnotation = @@ -156,7 +157,7 @@ export function DurationDistributionChart({ x0: selection[0], x1: selection[1], y0: 0, - y1: 100000, + y1: yAxisMaxDomain, }, details: 'selection', }, From 85b2ff24115973afb27e7328dc7b57cd26724164 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 11 Aug 2022 10:55:13 +0200 Subject: [PATCH 10/11] Tweak loading behavior of failed transactions correlations. --- ..._failed_transactions_correlations.test.tsx | 40 +++++++++++++------ .../use_failed_transactions_correlations.ts | 34 +++++++++++----- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx index 1464e6411d497..54d455519329c 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx +++ b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx @@ -174,10 +174,6 @@ describe('useFailedTransactionsCorrelations', () => { jest.advanceTimersByTime(50); await waitFor(() => expect(result.current.progress.loaded).toBe(0)); jest.advanceTimersByTime(100); - await waitFor(() => expect(result.current.progress.loaded).toBe(0)); - jest.advanceTimersByTime(100); - await waitFor(() => expect(result.current.progress.loaded).toBe(0)); - jest.advanceTimersByTime(100); await waitFor(() => expect(result.current.progress.loaded).toBe(0.05)); expect(result.current.progress).toEqual({ @@ -185,6 +181,28 @@ describe('useFailedTransactionsCorrelations', () => { isRunning: true, loaded: 0.05, }); + expect(result.current.response).toEqual({ + ccsWarning: false, + fieldStats: undefined, + errorHistogram: undefined, + failedTransactionsCorrelations: undefined, + overallHistogram: [ + { + doc_count: 1234, + key: 'the-key', + }, + ], + percentileThresholdValue: 1.234, + }); + + jest.advanceTimersByTime(100); + await waitFor(() => expect(result.current.progress.loaded).toBe(0.1)); + + expect(result.current.progress).toEqual({ + error: undefined, + isRunning: true, + loaded: 0.1, + }); expect(result.current.response).toEqual({ ccsWarning: false, fieldStats: undefined, @@ -205,23 +223,23 @@ describe('useFailedTransactionsCorrelations', () => { }); jest.advanceTimersByTime(100); - await waitFor(() => expect(result.current.progress.loaded).toBe(0.1)); + await waitFor(() => expect(result.current.progress.loaded).toBe(0.15)); // field candidates are an implementation detail and - // will not be exposed, it will just set loaded to 0.1. + // will not be exposed, it will just set loaded to 0.15. expect(result.current.progress).toEqual({ error: undefined, isRunning: true, - loaded: 0.1, + loaded: 0.15, }); jest.advanceTimersByTime(100); - await waitFor(() => expect(result.current.progress.loaded).toBe(1)); + await waitFor(() => expect(result.current.progress.loaded).toBe(0.9)); expect(result.current.progress).toEqual({ error: undefined, isRunning: true, - loaded: 1, + loaded: 0.9, }); expect(result.current.response).toEqual({ @@ -257,9 +275,7 @@ describe('useFailedTransactionsCorrelations', () => { }); jest.advanceTimersByTime(100); - await waitFor(() => - expect(result.current.response.fieldStats).toBeDefined() - ); + await waitFor(() => expect(result.current.progress.loaded).toBe(1)); expect(result.current.progress).toEqual({ error: undefined, diff --git a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts index 85246c07f5bb7..f9c365c539e0f 100644 --- a/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts +++ b/x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts @@ -37,9 +37,10 @@ import { useFetchParams } from './use_fetch_params'; // Overall progress is a float from 0 to 1. const LOADED_OVERALL_HISTOGRAM = 0.05; -const LOADED_FIELD_CANDIDATES = LOADED_OVERALL_HISTOGRAM + 0.05; +const LOADED_ERROR_HISTOGRAM = LOADED_OVERALL_HISTOGRAM + 0.05; +const LOADED_FIELD_CANDIDATES = LOADED_ERROR_HISTOGRAM + 0.05; const LOADED_DONE = 1; -const PROGRESS_STEP_P_VALUES = 0.9; +const PROGRESS_STEP_P_VALUES = 0.9 - LOADED_FIELD_CANDIDATES; export function useFailedTransactionsCorrelations() { const fetchParams = useFetchParams(); @@ -101,6 +102,10 @@ export function useFailedTransactionsCorrelations() { } ); + if (abortCtrl.current.signal.aborted) { + return; + } + const { overallHistogram, totalDocCount, @@ -109,6 +114,16 @@ export function useFailedTransactionsCorrelations() { durationMax, } = overallHistogramResponse; + responseUpdate.overallHistogram = overallHistogram; + responseUpdate.totalDocCount = totalDocCount; + responseUpdate.percentileThresholdValue = percentileThresholdValue; + + setResponse({ + ...responseUpdate, + loaded: LOADED_OVERALL_HISTOGRAM, + }); + setResponse.flush(); + const errorHistogramResponse = await callApmApi( 'POST /internal/apm/latency/overall_distribution/transactions', { @@ -132,20 +147,17 @@ export function useFailedTransactionsCorrelations() { } ); - const { overallHistogram: errorHistogram } = errorHistogramResponse; - - responseUpdate.errorHistogram = errorHistogram; - responseUpdate.overallHistogram = overallHistogram; - responseUpdate.totalDocCount = totalDocCount; - responseUpdate.percentileThresholdValue = percentileThresholdValue; - if (abortCtrl.current.signal.aborted) { return; } + const { overallHistogram: errorHistogram } = errorHistogramResponse; + + responseUpdate.errorHistogram = errorHistogram; + setResponse({ ...responseUpdate, - loaded: LOADED_OVERALL_HISTOGRAM, + loaded: LOADED_ERROR_HISTOGRAM, }); setResponse.flush(); @@ -295,7 +307,7 @@ export function useFailedTransactionsCorrelations() { const progress = useMemo( () => ({ error, - loaded, + loaded: Math.round(loaded * 100) / 100, isRunning, }), [error, loaded, isRunning] From d0a41b35e0a85c916fae1567c6e87d740b2520a0 Mon Sep 17 00:00:00 2001 From: Walter Rafelsberger Date: Thu, 11 Aug 2022 11:09:56 +0200 Subject: [PATCH 11/11] Revert jest test assertions. --- .../charts/duration_distribution_chart/index.test.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx index e9fa4aae02d7c..05cedbdc1e391 100644 --- a/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx +++ b/x-pack/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx @@ -30,12 +30,12 @@ describe('TransactionDistributionChart', () => { ).toEqual([ { doc_count: 10 }, { doc_count: 10 }, - { doc_count: 0.5 }, - { doc_count: 0.5 }, - { doc_count: 0.5 }, + { doc_count: 0.0001 }, + { doc_count: 0.0001 }, + { doc_count: 0.0001 }, { doc_count: 10 }, { doc_count: 10 }, - { doc_count: 0.5 }, + { doc_count: 0.0001 }, { doc_count: 10 }, { doc_count: 10 }, ]);