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

Jump to bottom

[APM] Metrics powered UI POC #66871

Closed
wants to merge 4 commits into from
+478 −104
Closed

[APM] Metrics powered UI POC #66871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb45b3a
[APM] Metrics powered UI POC
dgieselaar May 18, 2020
10cc497
Merge branch 'master' of github.com:elastic/kibana into metrics-ui-poc
dgieselaar Jun 4, 2020
0ebef5d
Use max size
dgieselaar Jun 8, 2020
f2714bf
Merge branch 'master' of github.com:elastic/kibana into metrics-ui-poc
dgieselaar Jun 8, 2020
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
1 change: 1 addition & 0 deletions x-pack/plugins/apm/common/elasticsearch_fieldnames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export const TRANSACTION_ID = 'transaction.id';
export const TRANSACTION_SAMPLED = 'transaction.sampled';
export const TRANSACTION_BREAKDOWN_COUNT = 'transaction.breakdown.count';
export const TRANSACTION_PAGE_URL = 'transaction.page.url';
export const TRANSACTION_DURATION_HISTOGRAM = 'transaction.duration.histogram';

export const TRACE_ID = 'trace.id';

Expand Down
41 changes: 25 additions & 16 deletions x-pack/plugins/apm/common/projections/services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,46 @@
import {
Setup,
SetupUIFilters,
SetupTimeRange
SetupTimeRange,
SetupHasTransactionDurationMetrics
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
} from '../../server/lib/helpers/setup_request';
import { SERVICE_NAME, PROCESSOR_EVENT } from '../elasticsearch_fieldnames';
import { SERVICE_NAME } from '../elasticsearch_fieldnames';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { rangeFilter } from '../../server/lib/helpers/range_filter';

export function getServicesProjection({
setup
}: {
setup: Setup & SetupTimeRange & SetupUIFilters;
setup: Setup &
SetupTimeRange &
SetupUIFilters &
SetupHasTransactionDurationMetrics;
}) {
const { start, end, uiFiltersES, indices } = setup;
const {
start,
end,
uiFiltersES,
indices,
hasTransactionDurationMetrics
Copy link
Member

Choose a reason for hiding this comment

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

Nit about the naming: Is it important to note that it's TransactionDuration docs, and not just Transaction docs?

What about hasPreAggregatedTransactions or isPreAggregationEnabled (if we in the future want to pre-aggregate other data types, eg. errors ?

} = setup;

const index = [
indices['apm_oss.metricsIndices'],
indices['apm_oss.errorIndices']
Copy link
Member

@sorenlouv sorenlouv May 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Are there any plans to pre-aggregate errors in the future?

];

if (!hasTransactionDurationMetrics) {
index.push(indices['apm_oss.transactionIndices']);
}
Copy link
Member

@sorenlouv sorenlouv May 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of having this for every query, what about letting the APM elasticsearch client handle this? So the consumer side specifies the indicies that should be queried as strings (typescript can enforce correctness):

{
  apmIndices: ['transactions', 'errors']
  body: {...}
}

Then the client will determine which indices should be queried according to hasTransactionDurationMetrics:

const indices = apmIndices.map(apmIndex => {
 switch(apmIndex) {
  case 'span': 
    return indices['apm_oss.spanIndices'];

  case 'error': 
    return indices['apm_oss.errorIndices'];

  case 'metric': 
    return indices['apm_oss.metricsIndices'];

  case 'transaction': 
    return hasTransactionDurationMetrics 
      ? indices['apm_oss.metricsIndices'] 
      : indices['apm_oss.transactionIndices'] 
 }
})

We need this client anyway to make it easier when teams outside APM needs want to query apm data (siem for instance)


return {
index: [
indices['apm_oss.metricsIndices'],
indices['apm_oss.errorIndices'],
indices['apm_oss.transactionIndices']
],
index,
body: {
size: 0,
query: {
bool: {
filter: [
{
terms: { [PROCESSOR_EVENT]: ['transaction', 'error', 'metric'] }
},
Copy link
Member

@sorenlouv sorenlouv May 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Removing the processor.event filter could cause problems because by default we query apm-* (all indices). So this query will return transactions, metrics, errors and spans by default.
I think you'll need the same logic as for the index.

I think the queried indicies and processor.event should always be aligned. I was hoping we could add the processor.event filter in the setup (middleware) but it might be better with an explicit helper. Something like:

const { index, processorEvent } = hasTransactionDurationMetrics
  ? // transactions are stored as pre-aggregated (histogram) docs in metric index
    getIndexAndProcessorEvent('metric', 'error')

  : // transactions are not pre-aggregated and stored in transaction index
    getIndexAndProcessorEvent('metric', 'error', transaction);

{ range: rangeFilter(start, end) },
...uiFiltersES
]
filter: [{ range: rangeFilter(start, end) }, ...uiFiltersES]
}
},
aggs: {
Expand Down
89 changes: 76 additions & 13 deletions x-pack/plugins/apm/server/lib/helpers/setup_request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
*/

import moment from 'moment';
import {
PROCESSOR_EVENT,
TRANSACTION_DURATION_HISTOGRAM
} from '../../../common/elasticsearch_fieldnames';
import { KibanaRequest } from '../../../../../../src/core/server';
import { IIndexPattern } from '../../../../../../src/plugins/data/common';
import { APMConfig } from '../..';
Expand All @@ -19,6 +23,7 @@ import { APMRequestHandlerContext } from '../../routes/typings';
import { getESClient } from './es_client';
import { ProcessorEvent } from '../../../common/processor_event';
import { getDynamicIndexPattern } from '../index_pattern/get_dynamic_index_pattern';
import { rangeFilter } from './range_filter';

function decodeUiFilters(
indexPattern: IIndexPattern | undefined,
Expand Down Expand Up @@ -50,6 +55,10 @@ export interface SetupUIFilters {
uiFiltersES: ESFilter[];
}

export interface SetupHasTransactionDurationMetrics {
hasTransactionDurationMetrics: boolean;
}

interface SetupRequestParams {
query?: {
_debug?: boolean;
Expand All @@ -60,17 +69,31 @@ interface SetupRequestParams {
};
}

type InferSetup<TParams extends SetupRequestParams> = Setup &
interface SetupRequestOptions {
checkForTransactionDurationMetrics: boolean;
}

type InferSetup<
TParams extends SetupRequestParams,
TOptions extends SetupRequestOptions
> = Setup &
(TParams extends { query: { start: string } } ? { start: number } : {}) &
(TParams extends { query: { end: string } } ? { end: number } : {}) &
(TParams extends { query: { uiFilters: string } }
? { uiFiltersES: ESFilter[] }
: {}) &
(TOptions extends { checkForTransactionDurationMetrics: true }
? { hasTransactionDurationMetrics: boolean }
: {});

export async function setupRequest<TParams extends SetupRequestParams>(
export async function setupRequest<
TParams extends SetupRequestParams,
TOptions extends SetupRequestOptions
>(
context: APMRequestHandlerContext<TParams>,
request: KibanaRequest
): Promise<InferSetup<TParams>> {
request: KibanaRequest,
options?: TOptions
): Promise<InferSetup<TParams, TOptions>> {
const { config } = context;
const { query } = context.params;

Expand All @@ -79,17 +102,56 @@ export async function setupRequest<TParams extends SetupRequestParams>(
config
});

const dynamicIndexPattern = await getDynamicIndexPattern({
context,
indices,
processorEvent: query.processorEvent
});
const client = getESClient(context, request, { clientAsInternalUser: false });

const start =
'start' in query ? { start: moment.utc(query.start).valueOf() } : {};
const end = 'end' in query ? { end: moment.utc(query.end).valueOf() } : {};
Comment on lines +107 to +109
Copy link
Member

Choose a reason for hiding this comment

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

What about:

Suggested change
const start =
'start' in query ? { start: moment.utc(query.start).valueOf() } : {};
const end = 'end' in query ? { end: moment.utc(query.end).valueOf() } : {};
const start = 'start' in query ? moment.utc(query.start).valueOf() : undefined;
const end = 'end' in query ? moment.utc(query.end).valueOf() : undefined;

Then you don't have to do start.start and end.end. I think start and end should always be returned, regardless if they are undefined


const checkTransactionDurationMetrics = async () => {
const response = await client.search({
index: indices['apm_oss.metricsIndices'],
body: {
query: {
bool: {
filter: [
{ term: { [PROCESSOR_EVENT]: 'metric' } },
...(start.start && end.end
? [{ range: rangeFilter(start.start, end.end) }]
: []),
{ exists: { field: TRANSACTION_DURATION_HISTOGRAM } }
]
}
},
size: 0
},
terminateAfter: 1
});

return {
hasTransactionDurationMetrics: response.hits.total.value > 0
};
};
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps extract this to separate file to avoid making setup_request too overwhelming


const [
dynamicIndexPattern,
hasTransactionDurationMetrics
] = await Promise.all([
getDynamicIndexPattern({
context,
indices,
processorEvent: query.processorEvent
}),
options?.checkForTransactionDurationMetrics
? checkTransactionDurationMetrics()
: Promise.resolve({})
]);

const uiFiltersES = decodeUiFilters(dynamicIndexPattern, query.uiFilters);

const coreSetupRequest = {
indices,
client: getESClient(context, request, { clientAsInternalUser: false }),
client,
internalClient: getESClient(context, request, {
clientAsInternalUser: true
}),
Expand All @@ -98,9 +160,10 @@ export async function setupRequest<TParams extends SetupRequestParams>(
};

return {
...('start' in query ? { start: moment.utc(query.start).valueOf() } : {}),
...('end' in query ? { end: moment.utc(query.end).valueOf() } : {}),
...start,
...end,
...hasTransactionDurationMetrics,
...('uiFilters' in query ? { uiFiltersES } : {}),
...coreSetupRequest
} as InferSetup<TParams>;
} as InferSetup<TParams, TOptions>;
}
Loading