Skip to content
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.

Sign up for GitHub

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

[ML] APM Correlations: Fix chart errors caused by inconsistent histogram range steps. #138259

Merged
merged 12 commits into from
Aug 11, 2022
Merged

[ML] APM Correlations: Fix chart errors caused by inconsistent histogram range steps. #138259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5f5d445
Fix range steps for latency correlations chart to be in sync.
walterra Aug 8, 2022
f8c3ef6
Fix duration overrides for latency correlations and faild transaction…
walterra Aug 8, 2022
b0c2e0d
[ML] Fix tests.
walterra Aug 8, 2022
35c5021
[ML] Make use of updated Elastic Charts available styles for orphaned…
walterra Aug 8, 2022
64fff29
[ML] Fix tests.
walterra Aug 8, 2022
4ad0a05
[ML] Add comment to clarify test structure.
walterra Aug 10, 2022
73241a7
Merge branch 'main' into fix-latency-correlation-range-steps
walterra Aug 11, 2022
cde87ac
Revert chart style to not show continuous line.
walterra Aug 11, 2022
52cf77e
Move the data transform to a useMemo.
walterra Aug 11, 2022
62db8d8
Set the height of the selection annotation dynamically based on the m…
walterra Aug 11, 2022
85b2ff2
Tweak loading behavior of failed transactions correlations.
walterra Aug 11, 2022
d0a41b3
Revert jest test assertions.
walterra Aug 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 31 additions & 10 deletions ...gins/apm/public/components/app/correlations/use_failed_transactions_correlations.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,41 @@ 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);
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({
error: undefined,
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,
Expand All @@ -200,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({
Expand Down Expand Up @@ -252,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,
Expand Down
117 changes: 70 additions & 47 deletions ...ck/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -85,61 +86,78 @@ 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,
},
},
}
);

if (abortCtrl.current.signal.aborted) {
return;
}

const { overallHistogram, totalDocCount, percentileThresholdValue } =
overallHistogramResponse;
const { overallHistogram: errorHistogram } = errorHistogramRespone;
const {
overallHistogram,
totalDocCount,
percentileThresholdValue,
durationMin,
durationMax,
} = overallHistogramResponse;

responseUpdate.errorHistogram = errorHistogram;
responseUpdate.overallHistogram = overallHistogram;
responseUpdate.totalDocCount = totalDocCount;
responseUpdate.percentileThresholdValue = percentileThresholdValue;

setResponse({
...responseUpdate,
loaded: LOADED_OVERALL_HISTOGRAM,
});
setResponse.flush();

const errorHistogramResponse = await callApmApi(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered not blocking this call? So we'd render the chart with the histogram and load the errors later? We could even add an indication that errors are still being fetched on the UI.

cc: @formgeist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I tweaked the loading behavior in 85b2ff2. After loading the overall histogram data we'll now already trigger an update that displays the chart with the available data and updates the progress bar. The updated jest tests also reflect the updated progress steps of the progress bar.

'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,
},
},
}
);

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();

Expand Down Expand Up @@ -179,7 +197,12 @@ export function useFailedTransactionsCorrelations() {
{
signal: abortCtrl.current.signal,
params: {
body: { ...fetchParams, fieldCandidates: fieldCandidatesChunk },
body: {
...fetchParams,
fieldCandidates: fieldCandidatesChunk,
durationMin,
durationMax,
},
},
}
);
Expand Down Expand Up @@ -284,7 +307,7 @@ export function useFailedTransactionsCorrelations() {
const progress = useMemo(
() => ({
error,
loaded,
loaded: Math.round(loaded * 100) / 100,
isRunning,
}),
[error, loaded, isRunning]
Expand Down
38 changes: 24 additions & 14 deletions x-pack/plugins/apm/public/components/app/correlations/use_latency_correlations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +89 to +103
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call is exactly the same as the one on x-pack/plugins/apm/public/components/app/correlations/use_failed_transactions_correlations.ts do you think it is worth extracting it and reusing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'd like to get this as a bugfix into 8.4 I'd like to avoid more refactoring, I added a comment to the meta issue in the tech debt section instead to follow up on it (Revisit hooks that fetch correlations results and possibly consolidate duplicate code like fetching the overall histogram.): #118009

},
}
);
},
}
);
responseUpdate.overallHistogram = overallHistogram;
responseUpdate.totalDocCount = totalDocCount;
responseUpdate.percentileThresholdValue = percentileThresholdValue;
Expand Down Expand Up @@ -192,7 +197,12 @@ export function useLatencyCorrelations() {
{
signal: abortCtrl.current.signal,
params: {
body: { ...fetchParams, fieldValuePairs: fieldValuePairChunk },
body: {
...fetchParams,
durationMin,
durationMax,
fieldValuePairs: fieldValuePairChunk,
},
},
}
);
Expand Down
8 changes: 6 additions & 2 deletions ...omponents/app/transaction_details/distribution/use_transaction_distribution_chart_data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,14 +86,18 @@ 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',
{
params: {
body: {
...params,
durationMin: overallLatencyData.durationMin,
durationMax: overallLatencyData.durationMax,
percentileThreshold: DEFAULT_PERCENTILE_THRESHOLD,
termFilters: [
{
Expand All @@ -108,7 +112,7 @@ export const useTransactionDistributionChartData = () => {
);
}
},
[params]
[params, overallLatencyData.durationMin, overallLatencyData.durationMax]
);

useEffect(() => {
Expand Down
10 changes: 6 additions & 4 deletions ...ck/plugins/apm/public/components/shared/charts/duration_distribution_chart/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand All @@ -25,7 +25,9 @@ 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 },
Expand Down
Loading