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

[APM] Fix missing ML data, and NaN issue #34333

Merged
merged 4 commits into from
Apr 2, 2019
Merged

[APM] Fix missing ML data, and NaN issue #34333

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
bc98f55
[APM] Fix missing ML data, and NaN issue
sorenlouv Apr 2, 2019
6e709e8
Fixed tests in setup_request
sorenlouv Apr 2, 2019
97e7729
Fix snapshots
sorenlouv Apr 2, 2019
2fd0960
Don’t remove first bucket
sorenlouv Apr 2, 2019
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
38 changes: 30 additions & 8 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 @@ -11,7 +11,7 @@ import {
SearchParams
} from 'elasticsearch';
import { Legacy } from 'kibana';
import { cloneDeep, has, set } from 'lodash';
import { cloneDeep, has, isString, set } from 'lodash';
import moment from 'moment';
import { OBSERVER_VERSION_MAJOR } from 'x-pack/plugins/apm/common/elasticsearch_fieldnames';

Expand Down Expand Up @@ -43,12 +43,32 @@ interface APMRequestQuery {
esFilterQuery: string;
}

function addFilterForLegacyData({
omitLegacyData = true,
...params
}: APMSearchParams): SearchParams {
function getApmIndices(config: Legacy.KibanaConfig) {
return [
config.get<string>('apm_oss.errorIndices'),
config.get<string>('apm_oss.metricsIndices'),
config.get<string>('apm_oss.sourcemapIndices'),
config.get<string>('apm_oss.transactionIndices')
];
}

function isApmIndex(apmIndices: string[], indexParam: SearchParams['index']) {
console.log({ apmIndices, indexParam });
sorenlouv marked this conversation as resolved.
Show resolved Hide resolved
if (isString(indexParam)) {
return apmIndices.includes(indexParam);
} else if (Array.isArray(indexParam)) {
Copy link
Member

Choose a reason for hiding this comment

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

When is this an array? And can it be an array of mixed (some apm and some non-apm)? Just wondering if that would end up breaking the UI with 6.x apm data leaking in…

Copy link
Member Author

@sorenlouv sorenlouv Apr 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

When is this an array?

It's an array when we want to query multiple indices, like here:

kibana/x-pack/plugins/apm/server/lib/services/get_service.ts

Lines 42 to 45 in d3dd95d

index: [
config.get<string>('apm_oss.errorIndices'),
config.get<string>('apm_oss.transactionIndices')
],

And can it be an array of mixed (some apm and some non-apm)?

We don't do that atm, and I don't think we will. That being said, I added a test for this exact scenario, and if any of the indices are not apm indices we will not add the filter. In that case 6.x data will leak in, but I think we'll have to cross that bridge when we get there. I don't know how else to do this, so unless you have another solution that takes that into account, I think we can leave it at that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the only thing I can think is to throw an error here if the indices are mixed, to alert us on the first time we try to do that so we remember this caveat. But that may be overkill if we expect this problem to sort of disappear as we get further and further past the 7.0 release...

Copy link
Member Author

Choose a reason for hiding this comment

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

Throwing an exception for mixed arrays would add additional logic - I'd prefer to not do that atm.

// return false if at least one of the indices is not an APM index
return indexParam.every(index => apmIndices.includes(index));
}
return false;
}

function addFilterForLegacyData(
apmIndices: string[],
{ omitLegacyData = true, ...params }: APMSearchParams
): SearchParams {
// search across all data (including data)
if (!omitLegacyData) {
if (!omitLegacyData || !isApmIndex(apmIndices, params.index)) {
return params;
}

Expand All @@ -69,12 +89,14 @@ export function setupRequest(req: Legacy.Request): Setup {
const query = (req.query as unknown) as APMRequestQuery;
const cluster = req.server.plugins.elasticsearch.getCluster('data');
const uiSettings = req.getUiSettingsService();
const config = req.server.config();
const apmIndices = getApmIndices(config);

const client: ESClient = async (type, params) => {
const includeFrozen = await uiSettings.get('search:includeFrozen');

const nextParams = {
...addFilterForLegacyData(params), // filter out pre-7.0 data
...addFilterForLegacyData(apmIndices, params), // filter out pre-7.0 data
ignore_throttled: !includeFrozen, // whether to query frozen indices or not
rest_total_hits_as_int: true // ensure that ES returns accurate hits.total with pre-6.6 format
};
Expand All @@ -99,6 +121,6 @@ export function setupRequest(req: Legacy.Request): Setup {
end: moment.utc(query.end).valueOf(),
esFilterQuery: decodeEsQuery(query.esFilterQuery),
client,
config: req.server.config()
config
};
}
4 changes: 2 additions & 2 deletions x-pack/plugins/apm/server/lib/transactions/charts/get_timeseries_data/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function getTpmBuckets(
) {
const buckets = transactionResultBuckets.map(
({ key: resultKey, timeseries }) => {
const dataPoints = timeseries.buckets.slice(1, -1).map(bucket => {
const dataPoints = timeseries.buckets.slice(1).map(bucket => {
return {
x: bucket.key,
y: round(bucket.doc_count * (60 / bucketSize), 1)
Expand All @@ -82,7 +82,7 @@ export function getTpmBuckets(
function getResponseTime(
responseTimeBuckets: ESResponse['aggregations']['response_times']['buckets'] = []
) {
return responseTimeBuckets.slice(1, -1).reduce(
return responseTimeBuckets.slice(1).reduce(
(acc, bucket) => {
const { '95.0': p95, '99.0': p99 } = bucket.pct.values;

sorenlouv marked this conversation as resolved.
Show resolved Hide resolved
Expand Down