-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Move to modified version of original status count for speed #41210
Changes from 5 commits
36cda82
48d106b
76e4ac2
56e97fb
bff50a3
ec236ce
a9ac880
a83a19d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -197,11 +197,10 @@ export class ElasticsearchMonitorsAdapter implements UMMonitorsAdapter { | |
aggs: { | ||
latest: { | ||
top_hits: { | ||
sort: [ | ||
{ | ||
'@timestamp': { order: 'desc' }, | ||
}, | ||
], | ||
sort: [{ '@timestamp': { order: 'desc' } }], | ||
_source: { | ||
includes: ['summary.*', 'monitor.id', '@timestamp', 'observer.geo.name'], | ||
}, | ||
size: 1, | ||
}, | ||
}, | ||
|
@@ -211,10 +210,16 @@ export class ElasticsearchMonitorsAdapter implements UMMonitorsAdapter { | |
}, | ||
}; | ||
|
||
let up: number = 0; | ||
let down: number = 0; | ||
let searchAfter: any = null; | ||
|
||
const summaryByIdLocation: { | ||
// ID | ||
[key: string]: { | ||
// Location | ||
[key: string]: { up: number; down: number; timestamp: number }; | ||
}; | ||
} = {}; | ||
|
||
do { | ||
if (searchAfter) { | ||
set(params, 'body.aggs.ids.composite.after', searchAfter); | ||
|
@@ -225,20 +230,67 @@ export class ElasticsearchMonitorsAdapter implements UMMonitorsAdapter { | |
|
||
idBuckets.forEach(bucket => { | ||
// We only get the latest doc | ||
const status = get(bucket, 'latest.hits.hits[0]._source.monitor.status', null); | ||
if (!statusFilter || (statusFilter && statusFilter === status)) { | ||
if (status === 'up') { | ||
up++; | ||
} else { | ||
down++; | ||
} | ||
const source: any = get(bucket, 'latest.hits.hits[0]._source'); | ||
const { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these keys guaranteed to be defined? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, aggregations always return a value. |
||
summary: { up, down }, | ||
monitor: { id }, | ||
} = source; | ||
const timestamp = source['@timestamp']; | ||
andrewvc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const location = get(source, 'observer.geo.name', ''); | ||
|
||
let idSummary = summaryByIdLocation[id]; | ||
if (!idSummary) { | ||
idSummary = {}; | ||
summaryByIdLocation[id] = idSummary; | ||
} | ||
const locationSummary = idSummary[location]; | ||
if (!locationSummary || locationSummary.timestamp < timestamp) { | ||
idSummary[location] = { timestamp, up, down }; | ||
} | ||
}); | ||
|
||
searchAfter = get(queryResult, 'aggregations.ids.after_key'); | ||
} while (searchAfter); | ||
|
||
return { up, down, total: up + down }; | ||
let up: number = 0; | ||
let mixed: number = 0; | ||
let down: number = 0; | ||
|
||
for (const id in summaryByIdLocation) { | ||
if (!summaryByIdLocation.hasOwnProperty(id)) { | ||
continue; | ||
} | ||
const locationInfo = summaryByIdLocation[id]; | ||
let locationUp = 0; | ||
let locationDown = 0; | ||
for (const locationName in locationInfo) { | ||
andrewvc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!locationInfo.hasOwnProperty(locationName)) { | ||
continue; | ||
} | ||
const locationData = locationInfo[locationName]; | ||
locationUp += locationData.up; | ||
locationDown += locationData.down; | ||
} | ||
|
||
if (locationDown === 0) { | ||
up++; | ||
} else if (locationUp > 0) { | ||
mixed++; | ||
} else { | ||
down++; | ||
} | ||
} | ||
|
||
const result: any = { up, down, mixed, total: up + down + mixed }; | ||
if (statusFilter) { | ||
for (const status in result) { | ||
if (status !== 'total' && status !== statusFilter) { | ||
result[status] = 0; | ||
} | ||
} | ||
} | ||
|
||
return result; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have a unit test for this function? It may be good to create a mock JSON result (from real-world query results) and a subsequent test snapshot to help ensure that future revisions don't break this function, since we're planning to make it GA and we've developed it so late. If you agree then I'm happy to add this, or do it in a follow-up PR. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this specific case I don't see a ton of value in the unit test. Our functional tests check this against real query results from an ES cluster. That said it'd be nice to have the unit test to make future development against this code easier, since running functional tests is a pain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might make more of a meta-style issue to try to address this. Once we've stabilized these backend functions it'll be important to ensure that future changes don't erode the functionality. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ to have a style discussion elsewhere. When/where to unit test is a complex question with no one single answer, and no shortage of different philosophies. |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"snapshot": { | ||
"counts": { "down": 2, "mixed": 0, "up": 0, "total": 2 } | ||
"counts": { "down": 2, "mixed": 0, "up": 0, "total": 10 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that we're doing this now. I think. 😺 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would categorize the previous behavior as a bug IMHO. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
{ | ||
"snapshot": { | ||
"counts": { "down": 0, "mixed": 0, "up": 8, "total": 8 } | ||
"counts": { "down": 0, "mixed": 0, "up": 8, "total": 10 } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably extract a type for this and put it in the adjacent
adapter_types
file. A separate type for the location value would probably be cleaner too.