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

[Resolver] aria-level and aria-flowto support enhancements #71777

Closed
wants to merge 12 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ export function eventName(event: ResolverEvent): string {
}
}

export function eventId(event: ResolverEvent): string {
export function eventId(event: ResolverEvent): number | undefined | string {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-buttner this function was converting the serial event id to a string. if it was undefined or 0 it was being converted to ''. i don't know the reasoning, but i undid it. some BE code was effected. let me know if this is OK

Copy link
Contributor

Choose a reason for hiding this comment

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

@bkimmel or @kqualters-elastic may have thoughts here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine I left some comments about checking for undefined and not displaying the string 'undefined'.

if (isLegacyEvent(event)) {
return event.endgame.serial_event_id ? String(event.endgame.serial_event_id) : '';
return event.endgame.serial_event_id;
}
return event.event.id;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { uniquePidForProcess, uniqueParentPidForProcess, datetime } from '../process_event';
import { uniquePidForProcess, uniqueParentPidForProcess, orderByTime } from '../process_event';
import { IndexedProcessTree } from '../../types';
import { ResolverEvent } from '../../../../common/endpoint/types';
import { levelOrder as baseLevelOrder } from '../../lib/tree_sequencers';
Expand Down Expand Up @@ -38,18 +38,7 @@ export function factory(

// sort the children of each node
for (const siblings of idToChildren.values()) {
siblings.sort(function (firstEvent, secondEvent) {
const first: number = datetime(firstEvent);
const second: number = datetime(secondEvent);

// if either value is NaN, compare them differently
if (isNaN(first)) {
// treat NaN as 1 and other values as 0, causing NaNs to have the highest value
return 1 - (isNaN(second) ? 1 : 0);
} else {
return first - second;
}
});
siblings.sort(orderByTime);
}

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { eventType } from './process_event';
import { eventType, orderByTime } from './process_event';

import { mockProcessEvent } from './process_event_test_helpers';
import { LegacyEndpointEvent } from '../../../common/endpoint/types';
import { LegacyEndpointEvent, ResolverEvent } from '../../../common/endpoint/types';

describe('process event', () => {
describe('eventType', () => {
Expand All @@ -24,4 +24,86 @@ describe('process event', () => {
expect(eventType(event)).toEqual('processCreated');
});
});
describe('orderByTime', () => {
let mock: (time: number, eventID: string) => ResolverEvent;
let events: ResolverEvent[];
beforeEach(() => {
mock = (time, eventID) => {
return {
'@timestamp': time,
event: {
id: eventID,
},
} as ResolverEvent;
};
// 2 events each for numbers -1, 0, 1, and NaN
// each event has a unique id, a through h
// order is arbitrary
events = [
mock(-1, 'a'),
mock(0, 'c'),
mock(1, 'e'),
mock(NaN, 'g'),
mock(-1, 'b'),
mock(0, 'd'),
mock(1, 'f'),
mock(NaN, 'h'),
];
});
it('sorts events as expected', () => {
events.sort(orderByTime);
expect(events).toMatchInlineSnapshot(`
Array [
Object {
"@timestamp": -1,
"event": Object {
"id": "a",
},
},
Object {
"@timestamp": -1,
"event": Object {
"id": "b",
},
},
Object {
"@timestamp": 0,
"event": Object {
"id": "c",
},
},
Object {
"@timestamp": 0,
"event": Object {
"id": "d",
},
},
Object {
"@timestamp": 1,
"event": Object {
"id": "e",
},
},
Object {
"@timestamp": 1,
"event": Object {
"id": "f",
},
},
Object {
"@timestamp": NaN,
"event": Object {
"id": "g",
},
},
Object {
"@timestamp": NaN,
"event": Object {
"id": "h",
},
},
]
`);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,13 @@ export function isTerminatedProcess(passedEvent: ResolverEvent) {
* ms since unix epoc, based on timestamp.
* may return NaN if the timestamp wasn't present or was invalid.
*/
export function datetime(passedEvent: ResolverEvent): number {
export function datetime(passedEvent: ResolverEvent): number | null {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelolo24 when the value isn't defined or can't be parsed this now returns null.

const timestamp = event.eventTimestamp(passedEvent);

return timestamp === undefined ? 0 : new Date(timestamp).getTime();
const time = timestamp === undefined ? 0 : new Date(timestamp).getTime();

// if the date could not be parsed, return null
return isNaN(time) ? null : time;
}

/**
Expand Down Expand Up @@ -171,3 +174,22 @@ export function argsForProcess(passedEvent: ResolverEvent): string | undefined {
}
return passedEvent?.process?.args;
}

/**
* used to sort events
*/
export function orderByTime(first: ResolverEvent, second: ResolverEvent): number {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-buttner this now does a tie breaker with an arbitrary comparison of event ID.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thanks!

const firstDatetime: number | null = datetime(first);
const secondDatetime: number | null = datetime(second);

if (firstDatetime === secondDatetime) {
// break ties using an arbitrary (stable) comparison of `eventId` (which should be unique)
return String(event.eventId(first)).localeCompare(String(event.eventId(second)));
} else if (firstDatetime === null || secondDatetime === null) {
// sort `null`'s as higher than numbers
return (firstDatetime === null ? 1 : 0) - (secondDatetime === null ? 1 : 0);
} else {
// sort in ascending order.
return firstDatetime - secondDatetime;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,9 @@ export const ProcessEventListNarrowedByType = memo(function ProcessEventListNarr
eventCategory: `${eventType}`,
eventType: `${event.ecsEventType(resolverEvent)}`,
name: event.descriptiveName(resolverEvent),
entityId,
entityId: String(entityId),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the behavior here is changed. Before, if the event was a legacy event and the id was undefined or 0this would be ''. Now it will be '0' or 'undefined'. I'm not sure that either behavior is correct.

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 undefined I wonder if we should mark it as an empty string here. I would think '0' would be fine if that is what was sent by the sensor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm also the field name here is a bit misleading, we're setting entityId with eventId. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went to track down what its used for. its not used. removing it.

setQueryParams: () => {
pushToQueryParams({ crumbId: entityId, crumbEvent: processEntityId });
pushToQueryParams({ crumbId: String(entityId), crumbEvent: processEntityId });
},
};
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export class PaginationBuilder {
const lastResult = results[results.length - 1];
const cursor = {
timestamp: lastResult['@timestamp'],
eventID: eventId(lastResult),
eventID: String(eventId(lastResult)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the behavior here is changed. Before, if the event was a legacy event and the id was undefined or 0this would be ''. Now it will be '0' or 'undefined'. I'm not sure that either behavior is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for point this out. I think we'll want to check if it is undefined and set it to an empty string here. If 0 is returned we'll likely want to keep it as 0 because I think that's a valid value for the legacy events.

};
return PaginationBuilder.urlEncodeCursor(cursor);
}
Expand Down