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.

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] Pagination of top 10 trace samples #51911

Merged
Merged
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

This file was deleted.

@@ -0,0 +1,62 @@
/*
* 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 { getFormattedBuckets } from '../index';

describe('Distribution', () => {
it('getFormattedBuckets', () => {
const buckets = [
{ key: 0, count: 0, samples: [] },
{ key: 20, count: 0, samples: [] },
{ key: 40, count: 0, samples: [] },
{
key: 60,
count: 5,
samples: [
{
transactionId: 'someTransactionId'
}
]
},
{
key: 80,
count: 100,
samples: [
{
transactionId: 'anotherTransactionId'
}
]
}
];
expect(getFormattedBuckets(buckets, 20)).toEqual([
{ x: 20, x0: 0, y: 0, style: { cursor: 'default' }, samples: [] },
{ x: 40, x0: 20, y: 0, style: { cursor: 'default' }, samples: [] },
{ x: 60, x0: 40, y: 0, style: { cursor: 'default' }, samples: [] },
{
x: 80,
x0: 60,
y: 5,
style: { cursor: 'pointer' },
samples: [
{
transactionId: 'someTransactionId'
}
]
},
{
x: 100,
x0: 80,
y: 100,
style: { cursor: 'pointer' },
samples: [
{
transactionId: 'anotherTransactionId'
}
]
}
]);
});
});
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Can you convert this to typescript while you are at it?

@@ -8,6 +8,7 @@ import { EuiIconTip, EuiTitle } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import d3 from 'd3';
import React, { FunctionComponent, useCallback } from 'react';
import { isEmpty } from 'lodash';
import { TransactionDistributionAPIResponse } from '../../../../../server/lib/transactions/distribution';
import { IBucket } from '../../../../../server/lib/transactions/distribution/get_buckets/transform';
import { IUrlParams } from '../../../../context/UrlParamsContext/types';
@@ -20,7 +21,7 @@ import { history } from '../../../../utils/history';
import { LoadingStatePrompt } from '../../../shared/LoadingStatePrompt';

interface IChartPoint {
sample?: IBucket['sample'];
samples: IBucket['samples'];
x0: number;
x: number;
y: number;
@@ -35,13 +36,15 @@ export function getFormattedBuckets(buckets: IBucket[], bucketSize: number) {
}

return buckets.map(
({ sample, count, key }): IChartPoint => {
({ samples, count, key }): IChartPoint => {
return {
sample,
samples,
x0: key,
x: key + bucketSize,
y: count,
style: { cursor: count > 0 && sample ? 'pointer' : 'default' }
style: {
cursor: count > 0 && !isEmpty(samples) ? 'pointer' : 'default'
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Do we even need to consider the count? Would this work?

Suggested change
cursor: count > 0 && !isEmpty(samples) ? 'pointer' : 'default'
cursor: !isEmpty(samples) ? 'pointer' : 'default'
}
};
}
);
@@ -91,6 +94,7 @@ interface Props {
distribution?: TransactionDistributionAPIResponse;
urlParams: IUrlParams;
isLoading: boolean;
onBucketSelected(index: number): void;
}

export const TransactionDistribution: FunctionComponent<Props> = (
@@ -99,7 +103,8 @@ export const TransactionDistribution: FunctionComponent<Props> = (
const {
distribution,
urlParams: { transactionId, traceId, transactionType },
isLoading
isLoading,
onBucketSelected
} = props;

const formatYShort = useCallback(getFormatYShort(transactionType), [
@@ -134,12 +139,15 @@ export const TransactionDistribution: FunctionComponent<Props> = (
const xMax = d3.max(buckets, d => d.x) || 0;
const timeFormatter = getDurationFormatter(xMax);

const bucketIndex = buckets.findIndex(
bucket =>
bucket.sample != null &&
bucket.sample.transactionId === transactionId &&
bucket.sample.traceId === traceId
);
const bucketIndex = buckets.findIndex(bucket => {
const sampleFound = bucket.samples.findIndex(
sample =>
sample.transactionId === transactionId && sample.traceId === traceId
);
return sampleFound !== -1;
});
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

You can simplify this with some:

  const bucketIndex = buckets.findIndex(bucket => {
    return bucket.samples.some(
      sample =>
        sample.transactionId === transactionId && sample.traceId === traceId
    );
  });

onBucketSelected(bucketIndex);
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Having side effects in render is normally a bad sign. This will call setSelectedBucket. Instead bucketIndex should be calculated outside TransactionDistribution and passed in.

<TransactionDistribution
    distribution={distributionData}
    isLoading={distributionStatus === FETCH_STATUS.LOADING}
    urlParams={urlParams}
    bucketIndex={bucketIndex}
/>

This comment has been minimized.

Copy link
@cauemarcondes

cauemarcondes Dec 2, 2019

Author Contributor

I wasn't totally sure if the logic to get the selected bucket was the responsibility of TransactionDistribution or TransactionDetail. But I totally agree with you that having no render effect is better.


return (
<div>
@@ -175,13 +183,14 @@ export const TransactionDistribution: FunctionComponent<Props> = (
bucketSize={distribution.bucketSize}
bucketIndex={bucketIndex}
onClick={(bucket: IChartPoint) => {
if (bucket.sample && bucket.y > 0) {
if (!isEmpty(bucket.samples) && bucket.y > 0) {
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Can this be simplified to:

Suggested change
if (!isEmpty(bucket.samples) && bucket.y > 0) {
if (!isEmpty(bucket.samples)) {

In other words: bucket.y should always be larger than 0 if the bucket has samples

const sample = bucket.samples[0];
history.push({
...history.location,
search: fromQuery({
...toQuery(history.location.search),
transactionId: bucket.sample.transactionId,
traceId: bucket.sample.traceId
transactionId: sample.transactionId,
traceId: sample.traceId
})
});
}
@@ -190,16 +199,18 @@ export const TransactionDistribution: FunctionComponent<Props> = (
formatYShort={formatYShort}
formatYLong={formatYLong}
verticalLineHover={(bucket: IChartPoint) =>
bucket.y > 0 && !bucket.sample
bucket.y > 0 && isEmpty(bucket.samples)
}
backgroundHover={(bucket: IChartPoint) =>
bucket.y > 0 && !isEmpty(bucket.samples)
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Perhaps?

Suggested change
bucket.y > 0 && !isEmpty(bucket.samples)
!isEmpty(bucket.samples)
}
backgroundHover={(bucket: IChartPoint) => bucket.y > 0 && bucket.sample}
tooltipHeader={(bucket: IChartPoint) => {
const xFormatted = timeFormatter(bucket.x);
const x0Formatted = timeFormatter(bucket.x0);
return `${x0Formatted.value} - ${xFormatted.value} ${xFormatted.unit}`;
}}
tooltipFooter={(bucket: IChartPoint) =>
!bucket.sample &&
isEmpty(bucket.samples) &&
i18n.translate(
'xpack.apm.transactionDetails.transactionsDurationDistributionChart.noSampleTooltip',
{
@@ -0,0 +1,87 @@
/*
* 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 React from 'react';
import { EuiButton, EuiFlexItem, EuiToolTip } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { Transaction as ITransaction } from '../../../../../typings/es_schemas/ui/Transaction';
import { TransactionDetailLink } from '../../../shared/Links/apm/TransactionDetailLink';
import { IWaterfall } from './WaterfallContainer/Waterfall/waterfall_helpers/waterfall_helpers';

export const MaybeViewTraceLink = ({
transaction,
waterfall
}: {
transaction: ITransaction;
waterfall: IWaterfall;
}) => {
const viewFullTraceButtonLabel = i18n.translate(
'xpack.apm.transactionDetails.viewFullTraceButtonLabel',
{
defaultMessage: 'View full trace'
}
);

// the traceroot cannot be found, so we cannot link to it
if (!waterfall.traceRoot) {
return (
<EuiFlexItem grow={false}>
<EuiToolTip
content={i18n.translate(
'xpack.apm.transactionDetails.noTraceParentButtonTooltip',
{
defaultMessage: 'The trace parent cannot be found'
}
)}
>
<EuiButton iconType="apmTrace" disabled={true}>
{viewFullTraceButtonLabel}
</EuiButton>
</EuiToolTip>
</EuiFlexItem>
);
}

const isRoot =
transaction.transaction.id === waterfall.traceRoot.transaction.id;

// the user is already viewing the full trace, so don't link to it
if (isRoot) {
return (
<EuiFlexItem grow={false}>
<EuiToolTip
content={i18n.translate(
'xpack.apm.transactionDetails.viewingFullTraceButtonTooltip',
{
defaultMessage: 'Currently viewing the full trace'
}
)}
>
<EuiButton iconType="apmTrace" disabled={true}>
{viewFullTraceButtonLabel}
</EuiButton>
</EuiToolTip>
</EuiFlexItem>
);

// the user is viewing a zoomed in version of the trace. Link to the full trace
} else {
const traceRoot = waterfall.traceRoot;
return (
<EuiFlexItem grow={false}>
<TransactionDetailLink
serviceName={traceRoot.service.name}
transactionId={traceRoot.transaction.id}
traceId={traceRoot.trace.id}
transactionName={traceRoot.transaction.name}
transactionType={traceRoot.transaction.type}
>
<EuiButton iconType="apmTrace">{viewFullTraceButtonLabel}</EuiButton>
</TransactionDetailLink>
</EuiFlexItem>
);
}
};
This conversation was marked as resolved by cauemarcondes

This comment has been minimized.

Copy link
@sqren

sqren Dec 2, 2019

Member

Good idea extracting this 👍

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.