diff --git a/.env.test b/.env.test index 567085ce..237fe292 100644 --- a/.env.test +++ b/.env.test @@ -38,3 +38,6 @@ REPORT_NOTIFY_URL=http://mock.com/ # Url for connecting to Redis REDIS_URL=redis://localhost:6379 + +# Disable memoization in tests +MEMOIZATION_TTL=-1 diff --git a/jest.config.js b/jest.config.js index 6887a5b2..fa2faf9c 100644 --- a/jest.config.js +++ b/jest.config.js @@ -28,6 +28,6 @@ module.exports = { setupFiles: [ './jest.setup.js' ], setupFilesAfterEnv: ['./jest.setup.redis-mock.js', './jest.setup.mongo-repl-set.js'], - + globalTeardown: './jest.global-teardown.js', }; diff --git a/jest.global-teardown.js b/jest.global-teardown.js index 4ee0b59a..447dfc0e 100644 --- a/jest.global-teardown.js +++ b/jest.global-teardown.js @@ -6,4 +6,4 @@ module.exports = () => { process.exit(0); }, 1000); } -} \ No newline at end of file +}; \ No newline at end of file diff --git a/lib/memoize/index.test.ts b/lib/memoize/index.test.ts index 359c5b1e..8953e00b 100644 --- a/lib/memoize/index.test.ts +++ b/lib/memoize/index.test.ts @@ -26,6 +26,7 @@ describe('memoize decorator — per-test inline classes', () => { @memoize({ strategy: 'concat', ttl: 60_000, max: 50 }) public async run(a: number, b: string) { this.calls += 1; + return `${a}-${b}`; } } @@ -37,7 +38,7 @@ describe('memoize decorator — per-test inline classes', () => { */ expect(await sample.run(1, 'x')).toBe('1-x'); /** - * In this case + * In this case */ expect(await sample.run(1, 'x')).toBe('1-x'); expect(await sample.run(1, 'x')).toBe('1-x'); @@ -52,6 +53,7 @@ describe('memoize decorator — per-test inline classes', () => { @memoize({ strategy: 'concat' }) public async run(a: unknown, b: unknown) { this.calls += 1; + return `${String(a)}|${String(b)}`; } } @@ -84,9 +86,11 @@ describe('memoize decorator — per-test inline classes', () => { it('should memoize return value for stringified objects across several calls', async () => { class Sample { public calls = 0; + @memoize({ strategy: 'concat' }) public async run(x: unknown, y: unknown) { this.calls += 1; + return 'ok'; } } @@ -103,9 +107,11 @@ describe('memoize decorator — per-test inline classes', () => { it('should memoize return value for method with non-default arguments (NaN, Infinity, -0, Symbol, Date, RegExp) still cache same-args', async () => { class Sample { public calls = 0; + @memoize({ strategy: 'concat' }) public async run(...args: unknown[]) { this.calls += 1; + return args.map(String).join(','); } } @@ -127,27 +133,31 @@ describe('memoize decorator — per-test inline classes', () => { class Sample { public calls = 0; + @memoize({ strategy: 'hash' }) public async run(...args: unknown[]) { this.calls += 1; + return 'ok'; } } const sample = new Sample(); - await sample.run({a: 1}, undefined, 0); - await sample.run({a: 1}, undefined, 0); + await sample.run({ a: 1 }, undefined, 0); + await sample.run({ a: 1 }, undefined, 0); - expect(hashSpy).toHaveBeenCalledWith([{a: 1}, undefined, 0], 'blake2b512', 'base64url'); + expect(hashSpy).toHaveBeenCalledWith([ { a: 1 }, undefined, 0], 'blake2b512', 'base64url'); expect(sample.calls).toBe(1); }); it('should not memoize return value with hash strategy and different arguments', async () => { class Sample { public calls = 0; + @memoize({ strategy: 'hash' }) public async run(...args: unknown[]) { this.calls += 1; + return 'ok'; } } @@ -163,9 +173,11 @@ describe('memoize decorator — per-test inline classes', () => { it('should memoize return value with hash strategy across several calls with same args', async () => { class Sample { public calls = 0; + @memoize({ strategy: 'hash' }) public async run(arg: unknown) { this.calls += 1; + return 'ok'; } } @@ -186,9 +198,11 @@ describe('memoize decorator — per-test inline classes', () => { class Sample { public calls = 0; + @memoizeWithMockedTimers({ strategy: 'concat', ttl: 1_000 }) public async run(x: string) { this.calls += 1; + return x; } } @@ -204,16 +218,19 @@ describe('memoize decorator — per-test inline classes', () => { await sample.run('k1'); expect(sample.calls).toBe(2); - }); it('error calls should never be momized', async () => { class Sample { public calls = 0; + @memoize() public async run(x: number) { this.calls += 1; - if (x === 1) throw new Error('boom'); + if (x === 1) { + throw new Error('boom'); + } + return x * 2; } } @@ -226,4 +243,87 @@ describe('memoize decorator — per-test inline classes', () => { await expect(sample.run(1)).rejects.toThrow('boom'); expect(sample.calls).toBe(2); }); + + it('should NOT cache results listed in skipCache (primitives)', async () => { + class Sample { + public calls = 0; + + @memoize({ strategy: 'concat', skipCache: [null, undefined, 0, false, ''] }) + public async run(kind: 'null' | 'undef' | 'zero' | 'false' | 'empty') { + this.calls += 1; + switch (kind) { + case 'null': return null; + case 'undef': return undefined; + case 'zero': return 0; + case 'false': return false; + case 'empty': return ''; + } + } + } + + const sample = new Sample(); + + // Each repeated call should invoke the original again because result is in skipCache. + await sample.run('null'); + await sample.run('null'); + + await sample.run('undef'); + await sample.run('undef'); + + await sample.run('zero'); + await sample.run('zero'); + + await sample.run('false'); + await sample.run('false'); + + await sample.run('empty'); + await sample.run('empty'); + + // 5 kinds × 2 calls each = 10 calls, none cached + expect(sample.calls).toBe(10); + }); + + it('should cache results NOT listed in skipCache', async () => { + class Sample { + public calls = 0; + + @memoize({ strategy: 'concat', skipCache: [null, undefined] }) + public async run(x: number) { + this.calls += 1; + // returns a non-skipped primitive + return x * 2; + } + } + + const sample = new Sample(); + + expect(await sample.run(21)).toBe(42); + expect(await sample.run(21)).toBe(42); + + expect(sample.calls).toBe(1); + }); + + it('should use equality for skipCache with objects: deep equal objects are cached', async () => { + const deepEqualObject = { a: 1 }; + + class Sample { + public calls = 0; + + @memoize({ strategy: 'concat', skipCache: [deepEqualObject] }) + public async run() { + this.calls += 1; + + return { a: 1 }; + } + } + + const sample = new Sample(); + + const first = await sample.run(); + const second = await sample.run(); + + expect(first).toEqual({ a: 1 }); + expect(second).toBe(first); + expect(sample.calls).toBe(1); + }); }); diff --git a/lib/memoize/index.ts b/lib/memoize/index.ts index 10430b69..a363360c 100644 --- a/lib/memoize/index.ts +++ b/lib/memoize/index.ts @@ -26,6 +26,11 @@ export interface MemoizeOptions { * Strategy for key generation */ strategy?: MemoizeKeyStrategy; + + /** + * It allows to skip caching for list of return values specified + */ + skipCache?: any[] } /** @@ -40,6 +45,7 @@ export function memoize(options: MemoizeOptions = {}): MethodDecorator { max = 50, ttl = 1000 * 60 * 30, strategy = 'concat', + skipCache = [] } = options; /* eslint-enable */ @@ -84,7 +90,9 @@ export function memoize(options: MemoizeOptions = {}): MethodDecorator { try { const result = await originalMethod.apply(this, args); - cache.set(key, result); + if (!skipCache.includes(result)) { + cache.set(key, result); + } return result; } catch (err) { diff --git a/workers/grouper/src/index.ts b/workers/grouper/src/index.ts index d8869e16..8eef04fc 100644 --- a/workers/grouper/src/index.ts +++ b/workers/grouper/src/index.ts @@ -13,8 +13,7 @@ import type { BacktraceFrame, SourceCodeLine, ProjectEventGroupingPatternsDBScheme, - ErrorsCatcherType, - CatcherMessagePayload + ErrorsCatcherType } from '@hawk.so/types'; import type { RepetitionDBScheme } from '../types/repetition'; import { DatabaseReadWriteError, DiffCalculationError, ValidationError } from '../../../lib/workerErrors'; @@ -23,9 +22,16 @@ import { MS_IN_SEC } from '../../../lib/utils/consts'; import DataFilter from './data-filter'; import RedisHelper from './redisHelper'; import { computeDelta } from './utils/repetitionDiff'; -import TimeMs from '../../../lib/utils/time'; import { rightTrim } from '../../../lib/utils/string'; import { hasValue } from '../../../lib/utils/hasValue'; +/* eslint-disable-next-line no-unused-vars */ +import { memoize } from '../../../lib/memoize'; + +/** + * eslint does not count decorators as a variable usage + */ +/* eslint-disable-next-line no-unused-vars */ +const MEMOIZATION_TTL = Number(process.env.MEMOIZATION_TTL ?? 0); /** * Error code of MongoDB key duplication error @@ -106,36 +112,42 @@ export default class GrouperWorker extends Worker { if (task.payload && task.payload.release !== undefined) { task.payload = { ...task.payload, - release: String(task.payload.release) - } + release: String(task.payload.release), + }; } - /** - * Find event by group hash. - */ - let existedEvent = await this.getEvent(task.projectId, uniqueEventHash); + let existedEvent: GroupedEventDBScheme; /** - * If we couldn't group by group hash (title), try grouping by patterns + * Find similar events by grouping pattern */ - if (!existedEvent) { - const similarEvent = await this.findSimilarEvent(task.projectId, task.payload); + const similarEvent = await this.findSimilarEvent(task.projectId, task.payload.title); - if (similarEvent) { - this.logger.info(`similar event: ${JSON.stringify(similarEvent)}`); - /** - * Override group hash with found event's group hash - */ - uniqueEventHash = similarEvent.groupHash; + if (similarEvent) { + this.logger.info(`similar event: ${JSON.stringify(similarEvent)}`); - existedEvent = similarEvent; - } + /** + * Override group hash with found event's group hash + */ + uniqueEventHash = similarEvent.groupHash; + + existedEvent = similarEvent; + } + + /** + * If we couldn't group by grouping pattern — try grouping bt hash (title) + */ + else { + /** + * Find event by group hash. + */ + existedEvent = await this.getEvent(task.projectId, uniqueEventHash); } /** * Event happened for the first time */ - const isFirstOccurrence = existedEvent === null; + const isFirstOccurrence = !existedEvent && !similarEvent; let repetitionId = null; @@ -281,6 +293,13 @@ export default class GrouperWorker extends Worker { }; }); }); + + /** + * Normalize backtrace, if backtrace equals to [] it leads to visual bugs + */ + if (event.backtrace.length === 0) { + event.backtrace = null; + } } /** @@ -296,31 +315,39 @@ export default class GrouperWorker extends Worker { }); } + /** + * Method that is used to retrieve the first original event that satisfies the grouping pattern + * + * @param pattern - event should satisfy this pattern + * @param projectId - id of the project to find event in + */ + private async findFirstEventByPattern(pattern: string, projectId: string): Promise { + return await this.eventsDb.getConnection() + .collection(`events:${projectId}`) + .findOne( + { 'payload.title': { $regex: pattern } } + ); + } + /** * Tries to find events with a small Levenshtein distance of a title or by matching grouping patterns * * @param projectId - where to find - * @param event - event to compare + * @param title - title of the event to find similar one */ - private async findSimilarEvent(projectId: string, event: EventData): Promise { + @memoize({ max: 200, ttl: MEMOIZATION_TTL, strategy: 'hash', skipCache: [undefined] }) + private async findSimilarEvent(projectId: string, title: string): Promise { /** * If no match by Levenshtein, try matching by patterns */ const patterns = await this.getProjectPatterns(projectId); if (patterns && patterns.length > 0) { - const matchingPattern = await this.findMatchingPattern(patterns, event); + const matchingPattern = await this.findMatchingPattern(patterns, title); if (matchingPattern !== null && matchingPattern !== undefined) { try { - const originalEvent = await this.cache.get(`${projectId}:${matchingPattern._id}:originalEvent`, async () => { - return await this.eventsDb.getConnection() - .collection(`events:${projectId}`) - .findOne( - { 'payload.title': { $regex: matchingPattern.pattern } }, - { sort: { _id: 1 } } - ); - }); + const originalEvent = await this.findFirstEventByPattern(matchingPattern.pattern, projectId); this.logger.info(`original event for pattern: ${JSON.stringify(originalEvent)}`); @@ -340,12 +367,12 @@ export default class GrouperWorker extends Worker { * Method that returns matched pattern for event, if event do not match any of patterns return null * * @param patterns - list of the patterns of the related project - * @param event - event which title would be cheched + * @param title - title of the event to check for pattern match * @returns {ProjectEventGroupingPatternsDBScheme | null} matched pattern object or null if no match */ private async findMatchingPattern( patterns: ProjectEventGroupingPatternsDBScheme[], - event: CatcherMessagePayload + title: string ): Promise { if (!patterns || patterns.length === 0) { return null; @@ -354,7 +381,7 @@ export default class GrouperWorker extends Worker { return patterns.filter(pattern => { const patternRegExp = new RegExp(pattern.pattern); - return event.title.match(patternRegExp); + return title.match(patternRegExp); }).pop() || null; } @@ -365,20 +392,13 @@ export default class GrouperWorker extends Worker { * @returns {ProjectEventGroupingPatternsDBScheme[]} EventPatterns object with projectId and list of patterns */ private async getProjectPatterns(projectId: string): Promise { - return this.cache.get(`project:${projectId}:patterns`, async () => { - const project = await this.accountsDb.getConnection() - .collection('projects') - .findOne({ - _id: new mongodb.ObjectId(projectId), - }); + const project = await this.accountsDb.getConnection() + .collection('projects') + .findOne({ + _id: new mongodb.ObjectId(projectId), + }); - return project?.eventGroupingPatterns || []; - }, - /** - * Cache project patterns for 5 minutes since they don't change frequently - */ - /* eslint-disable-next-line @typescript-eslint/no-magic-numbers */ - 5 * TimeMs.MINUTE / MS_IN_SEC); + return project?.eventGroupingPatterns || []; } /** diff --git a/workers/grouper/tests/index.test.ts b/workers/grouper/tests/index.test.ts index 6d1c9adc..153a7952 100644 --- a/workers/grouper/tests/index.test.ts +++ b/workers/grouper/tests/index.test.ts @@ -587,6 +587,81 @@ describe('GrouperWorker', () => { expect(await repetitionsCollection.find().count()).toBe(1); }); }); + + describe('dynamic pattern addition', () => { + test('should group events when pattern added after we receive the first event', async () => { + /** + * Remove all existing patterns from the project + */ + jest.spyOn(GrouperWorker.prototype as any, 'getProjectPatterns').mockResolvedValue([]); + + /** + * Two nearly identical titles that could be grouped by `New error .*` pattern + */ + const firstTitle = 'Dynamic pattern error 1111111111111111'; + const secondTitle = 'Dynamic pattern error 2222222222222222'; + + await worker.handle(generateTask({ title: firstTitle })); + await worker.handle(generateTask({ title: secondTitle })); + + const originalsBefore = await eventsCollection.find().toArray(); + + expect(originalsBefore.length).toBe(2); + + const originalA = originalsBefore.find(e => e.payload.title === firstTitle)!; + const originalB = originalsBefore.find(e => e.payload.title === secondTitle)!; + + expect(originalA).toBeTruthy(); + expect(originalB).toBeTruthy(); + + /** + * Two events should be stored separately since grouping patterns of the project were empty + */ + expect(originalA.groupHash).not.toBe(originalB.groupHash); + + jest.spyOn(GrouperWorker.prototype as any, 'getProjectPatterns').mockResolvedValue([ + { + _id: new mongodb.ObjectId(), + pattern: 'Dynamic pattern error .*', + }, + ]); + + /** + * Second title should be grouped with first event that matches inserted grouping pattern + * It should not be grouped with the existing event with same item because it violates grouping pattern logic + */ + await worker.handle(generateTask({ title: secondTitle })); + + const allEvents = await eventsCollection.find().toArray(); + const allRepetitions = await repetitionsCollection.find().toArray(); + + /** + * Should still be only 2 original event documents in the DB + */ + expect(allEvents.length).toBe(2); + + const refreshedOriginalA = await eventsCollection.findOne({ _id: originalA._id }); + const refreshedOriginalB = await eventsCollection.findOne({ _id: originalB._id }); + + // totalCount: originalA should have 2 (1 original + 1 new repetition), + // originalB should remain 1. + expect(refreshedOriginalA?.totalCount).toBe(2); + expect(refreshedOriginalB?.totalCount).toBe(1); + + // Repetitions should be 1 and must reference originalA's groupHash + expect(allRepetitions.length).toBe(1); + allRepetitions.forEach(rep => { + expect(rep.groupHash).toBe(refreshedOriginalA!.groupHash); + }); + + /** + * Original B should have zero repetitions despite same title with latest event passed + */ + const repsForOriginalB = await repetitionsCollection.find({ groupHash: refreshedOriginalB!.groupHash }).count(); + + expect(repsForOriginalB).toBe(0); + }); + }); }); describe('Event marks handling', () => { @@ -598,10 +673,12 @@ describe('GrouperWorker', () => { // Create an event first const firstTask = generateTask({ title: 'Test ignored event' }); + await worker.handle(firstTask); // Mark the event as ignored by updating it in database const eventHash = await (worker as any).getUniqueEventHash(firstTask); + await eventsCollection.updateOne( { groupHash: eventHash }, { $set: { marks: { ignored: true } } } @@ -609,6 +686,7 @@ describe('GrouperWorker', () => { // Handle the same event again (repetition) const secondTask = generateTask({ title: 'Test ignored event' }); + await worker.handle(secondTask); // Verify that addTask was called only once (for the first occurrence) @@ -624,10 +702,12 @@ describe('GrouperWorker', () => { // Create an event first const firstTask = generateTask({ title: 'Test non-ignored event' }); + await worker.handle(firstTask); // Handle the same event again (repetition) - without marking as ignored const secondTask = generateTask({ title: 'Test non-ignored event' }); + await worker.handle(secondTask); // Verify that addTask was called twice (for both occurrences) @@ -643,6 +723,7 @@ describe('GrouperWorker', () => { // Create a new event (first occurrence) const task = generateTask({ title: 'Test new event without marks' }); + await worker.handle(task); // Verify that addTask was called for the first occurrence