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

[Uptime] Fix race on overview page query #67843

Merged
merged 9 commits into from
Jun 9, 2020
Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* 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 { fullyMatchingIds } from '../refine_potential_matches';
import { MonitorLocCheckGroup } from '..';

const mockQueryResult = (opts: { latestSummary: any; latestMatching: any }) => {
return {
aggregations: {
monitor: {
buckets: [
{
key: 'my-monitor',
location: {
buckets: [
{
key: 'my-location',
summaries: {
latest: {
hits: {
hits: [
{
_source: opts.latestSummary,
},
],
},
},
},
latest_matching: {
top: {
hits: {
hits: [
{
_source: opts.latestMatching,
},
],
},
},
},
},
],
},
},
],
},
},
};
};

describe('fully matching IDs', () => {
it('should exclude items whose latest result does not match', () => {
const queryRes = mockQueryResult({
latestSummary: {
'@timestamp': '2020-06-04T12:39:54.698-0500',
monitor: {
check_group: 'latest-summary-check-group',
},
summary: {
up: 1,
down: 0,
},
},
latestMatching: {
'@timestamp': '2019-06-04T12:39:54.698-0500',
summary: {
up: 1,
down: 0,
},
},
});
const res = fullyMatchingIds(queryRes, undefined);
const expected = new Map<string, MonitorLocCheckGroup[]>();
expect(res).toEqual(expected);
});

it('should include items whose latest result does match', () => {
const queryRes = mockQueryResult({
latestSummary: {
'@timestamp': '2020-06-04T12:39:54.698-0500',
monitor: {
check_group: 'latest-summary-check-group',
},
summary: {
up: 1,
down: 0,
},
},
latestMatching: {
'@timestamp': '2020-06-04T12:39:54.698-0500',
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't you try increasing the timestamp of latestMatching to cover the use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept this case and added one for a future 'latest' timestamp

summary: {
up: 1,
down: 0,
},
},
});
const res = fullyMatchingIds(queryRes, undefined);
const expected = new Map<string, MonitorLocCheckGroup[]>();
expected.set('my-monitor', [
{
checkGroup: 'latest-summary-check-group',
location: 'my-location',
monitorId: 'my-monitor',
status: 'up',
summaryTimestamp: new Date('2020-06-04T12:39:54.698-0500'),
},
]);
expect(res).toEqual(expected);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ export const fetchChunk: ChunkFetcher = async (
searchAfter: any,
size: number
): Promise<ChunkResult> => {
const { monitorIds, checkGroups, searchAfter: foundSearchAfter } = await findPotentialMatches(
const { monitorIds, searchAfter: foundSearchAfter } = await findPotentialMatches(
queryContext,
searchAfter,
size
);
const matching = await refinePotentialMatches(queryContext, monitorIds, checkGroups);
const matching = await refinePotentialMatches(queryContext, monitorIds);

return {
monitorGroups: matching,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,8 @@ import { get, set } from 'lodash';
import { CursorDirection } from '../../../../common/runtime_types';
import { QueryContext } from './query_context';

// This is the first phase of the query. In it, we find the most recent check groups that matched the given query.
// Note that these check groups may not be the most recent groups for the matching monitor ID! We'll filter those
/**
* This is the first phase of the query. In it, we find the most recent check groups that matched the given query.
* Note that these check groups may not be the most recent groups for the matching monitor ID. They'll be filtered
* out in the next phase.
* This is the first phase of the query. In it, we find all monitor IDs that have ever matched the given filters.
* @param queryContext the data and resources needed to perform the query
* @param searchAfter indicates where Elasticsearch should continue querying on subsequent requests, if at all
* @param size the minimum size of the matches to chunk
Expand All @@ -24,29 +20,14 @@ export const findPotentialMatches = async (
size: number
) => {
const queryResult = await query(queryContext, searchAfter, size);
const checkGroups = new Set<string>();
const monitorIds: string[] = [];
get<any>(queryResult, 'aggregations.monitors.buckets', []).forEach((b: any) => {
const monitorId = b.key.monitor_id;
monitorIds.push(monitorId);

// Doc count can be zero if status filter optimization does not match
if (b.doc_count > 0) {
// Here we grab the most recent 2 check groups per location and add them to the list.
// Why 2? Because the most recent one may be a partial result from mode: all, and hence not match a summary doc.
b.locations.buckets.forEach((lb: any) => {
lb.ips.buckets.forEach((ib: any) => {
ib.top.hits.hits.forEach((h: any) => {
checkGroups.add(h._source.monitor.check_group);
});
});
});
}
});

return {
monitorIds,
checkGroups,
searchAfter: queryResult.aggregations?.monitors?.after_key,
};
};
Expand Down Expand Up @@ -89,29 +70,6 @@ const queryBody = async (queryContext: QueryContext, searchAfter: any, size: num
},
],
},
aggs: {
// Here we grab the most recent 2 check groups per location.
// Why 2? Because the most recent one may not be for a summary, it may be incomplete.
locations: {
terms: { field: 'observer.geo.name', missing: '__missing__' },
aggs: {
ips: {
terms: { field: 'monitor.ip', missing: '0.0.0.0' },
aggs: {
top: {
top_hits: {
sort: [{ '@timestamp': 'desc' }],
_source: {
includes: ['monitor.check_group', '@timestamp'],
},
size: 2,
},
},
},
},
},
},
},
},
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,14 @@ import { MonitorGroups, MonitorLocCheckGroup } from './fetch_page';
// check groups for their associated monitor IDs. If not, it discards the result.
export const refinePotentialMatches = async (
queryContext: QueryContext,
potentialMatchMonitorIDs: string[],
potentialMatchCheckGroups: Set<string>
potentialMatchMonitorIDs: string[]
): Promise<MonitorGroups[]> => {
if (potentialMatchMonitorIDs.length === 0) {
return [];
}

const recentGroupsMatchingStatus = await fullyMatchingIds(
queryContext,
potentialMatchMonitorIDs,
potentialMatchCheckGroups
);
const queryResult = await query(queryContext, potentialMatchMonitorIDs);
const recentGroupsMatchingStatus = await fullyMatchingIds(queryResult, queryContext.statusFilter);

// Return the monitor groups filtering out potential matches that weren't current
const matches: MonitorGroups[] = potentialMatchMonitorIDs
Expand All @@ -49,27 +45,32 @@ export const refinePotentialMatches = async (
return matches;
};

const fullyMatchingIds = async (
queryContext: QueryContext,
potentialMatchMonitorIDs: string[],
potentialMatchCheckGroups: Set<string>
) => {
const mostRecentQueryResult = await mostRecentCheckGroups(queryContext, potentialMatchMonitorIDs);

export const fullyMatchingIds = (queryResult: any, statusFilter?: string) => {
const matching = new Map<string, MonitorLocCheckGroup[]>();
MonitorLoop: for (const monBucket of mostRecentQueryResult.aggregations.monitor.buckets) {
MonitorLoop: for (const monBucket of queryResult.aggregations.monitor.buckets) {
const monitorId: string = monBucket.key;
const groups: MonitorLocCheckGroup[] = [];

// Did at least one location match?
let matched = false;
for (const locBucket of monBucket.location.buckets) {
const location = locBucket.key;
const topSource = locBucket.top.hits.hits[0]._source;
const checkGroup = topSource.monitor.check_group;
const status = topSource.summary.down > 0 ? 'down' : 'up';
const latestSource = locBucket.summaries.latest.hits.hits[0]._source;
const latestStillMatchingSource = locBucket.latest_matching.top.hits.hits[0]?._source;
// If the most recent document still matches the most recent document matching the current filters
// we can include this in the result
//
// We just check if the timestamp is greater. Note this may match an incomplete check group
// that has not yet sent a summary doc
if (latestStillMatchingSource['@timestamp'] >= latestSource['@timestamp']) {
matched = true;
}
const checkGroup = latestSource.monitor.check_group;
const status = latestSource.summary.down > 0 ? 'down' : 'up';

// This monitor doesn't match, so just skip ahead and don't add it to the output
// Only skip in case of up statusFilter, for a monitor to be up, all checks should be up
if (queryContext?.statusFilter === 'up' && queryContext.statusFilter !== status) {
if (statusFilter === 'up' && statusFilter !== status) {
continue MonitorLoop;
}

Expand All @@ -78,20 +79,20 @@ const fullyMatchingIds = async (
location,
checkGroup,
status,
summaryTimestamp: topSource['@timestamp'],
summaryTimestamp: new Date(latestSource['@timestamp']),
});
}

// We only truly match the monitor if one of the most recent check groups was found in the potential matches phase
if (groups.some((g) => potentialMatchCheckGroups.has(g.checkGroup))) {
// If one location matched, include data from all locations in the result set
if (matched) {
matching.set(monitorId, groups);
}
}

return matching;
};

export const mostRecentCheckGroups = async (
export const query = async (
queryContext: QueryContext,
potentialMatchMonitorIDs: string[]
): Promise<any> => {
Expand All @@ -104,8 +105,6 @@ export const mostRecentCheckGroups = async (
filter: [
await queryContext.dateRangeFilter(),
{ terms: { 'monitor.id': potentialMatchMonitorIDs } },
// only match summary docs because we only want the latest *complete* check group.
{ exists: { field: 'summary' } },
],
},
},
Expand All @@ -116,13 +115,39 @@ export const mostRecentCheckGroups = async (
location: {
terms: { field: 'observer.geo.name', missing: 'N/A', size: 100 },
aggs: {
top: {
top_hits: {
sort: [{ '@timestamp': 'desc' }],
_source: {
includes: ['monitor.check_group', '@timestamp', 'summary.up', 'summary.down'],
summaries: {
// only match summary docs because we only want the latest *complete* check group.
filter: { exists: { field: 'summary' } },
shahzad31 marked this conversation as resolved.
Show resolved Hide resolved
aggs: {
latest: {
top_hits: {
sort: [{ '@timestamp': 'desc' }],
_source: {
includes: [
'monitor.check_group',
'@timestamp',
'summary.up',
'summary.down',
],
},
size: 1,
},
},
},
},
// We want to find the latest check group, even if it's not part of a summary
latest_matching: {
shahzad31 marked this conversation as resolved.
Show resolved Hide resolved
filter: queryContext.filterClause || { match_all: {} },
aggs: {
top: {
top_hits: {
sort: [{ '@timestamp': 'desc' }],
_source: {
includes: ['monitor.check_group', '@timestamp'],
},
size: 1,
},
},
size: 1,
},
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export default function ({ getService }: FtrProviderContext) {
const url = getBaseUrl(dateRangeStart, dateRangeEnd) + `&filters=${filters}`;
const apiResponse = await supertest.get(url);
const nonSummaryRes = apiResponse.body;
// expect(JSON.stringify(nonSummaryRes)).to.eql("foo");
Copy link
Contributor

Choose a reason for hiding this comment

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

if it's not applicable anymore, it's better to remove 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.

My mistake, left over from debugging!

expect(nonSummaryRes.summaries.length).to.eql(1);
});

Expand Down