From bd541b4c2d1676a79e579d22c1eb0fade66d10c7 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Thu, 7 Apr 2022 10:02:19 +0100 Subject: [PATCH] Merge pull request #7347 from overleaf/bg-record-snapshot-metrics record metrics for docupdater range operations GitOrigin-RevId: 41c500630dd81670d2798d388f2961a19df76b63 --- .../document-updater/app/js/RangesManager.js | 29 ++++++-- .../document-updater/app/js/UpdateManager.js | 1 + .../js/RangesManager/RangesManagerTests.js | 70 ++++++++++++++++++- .../js/UpdateManager/UpdateManagerTests.js | 5 ++ 4 files changed, 97 insertions(+), 8 deletions(-) diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 25b44859c7..defba60e24 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -13,8 +13,11 @@ let RangesManager const RangesTracker = require('./RangesTracker') const logger = require('@overleaf/logger') +const Metrics = require('./Metrics') const _ = require('lodash') +const RANGE_DELTA_BUCKETS = [0, 1, 2, 3, 4, 5, 10, 20, 50] + module.exports = RangesManager = { MAX_COMMENTS: 500, MAX_CHANGES: 2000, @@ -32,7 +35,8 @@ module.exports = RangesManager = { } const { changes, comments } = _.cloneDeep(entries) const rangesTracker = new RangesTracker(changes, comments) - const emptyRangeCountBefore = RangesManager._emptyRangesCount(rangesTracker) + const [emptyRangeCountBefore, totalRangeCountBefore] = + RangesManager._emptyRangesCount(rangesTracker) for (const update of Array.from(updates)) { rangesTracker.track_changes = !!update.meta.tc if (update.meta.tc) { @@ -74,8 +78,18 @@ module.exports = RangesManager = { return callback(error) } - const emptyRangeCountAfter = RangesManager._emptyRangesCount(rangesTracker) + const [emptyRangeCountAfter, totalRangeCountAfter] = + RangesManager._emptyRangesCount(rangesTracker) const rangesWereCollapsed = emptyRangeCountAfter > emptyRangeCountBefore + // monitor the change in range count, we may want to snapshot before large decreases + if (totalRangeCountAfter < totalRangeCountBefore) { + Metrics.histogram( + 'range-delta', + totalRangeCountBefore - totalRangeCountAfter, + RANGE_DELTA_BUCKETS, + { status_code: rangesWereCollapsed ? 'saved' : 'unsaved' } + ) + } const response = RangesManager._getRanges(rangesTracker) logger.debug( { @@ -144,19 +158,22 @@ module.exports = RangesManager = { }, _emptyRangesCount(ranges) { - let count = 0 + let emptyCount = 0 + let totalCount = 0 for (const comment of Array.from(ranges.comments || [])) { + totalCount++ if (comment.op.c === '') { - count++ + emptyCount++ } } for (const change of Array.from(ranges.changes || [])) { + totalCount++ if (change.op.i != null) { if (change.op.i === '') { - count++ + emptyCount++ } } } - return count + return [emptyCount, totalCount] }, } diff --git a/services/document-updater/app/js/UpdateManager.js b/services/document-updater/app/js/UpdateManager.js index 1d8dff2d8d..77cb974abc 100644 --- a/services/document-updater/app/js/UpdateManager.js +++ b/services/document-updater/app/js/UpdateManager.js @@ -239,6 +239,7 @@ module.exports = UpdateManager = { return callback(error) } if (ranges_were_collapsed) { + Metrics.inc('doc-snapshot') logger.debug( { project_id, diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 56bb0cd03b..3189728036 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -18,7 +18,9 @@ const SandboxedModule = require('sandboxed-module') describe('RangesManager', function () { beforeEach(function () { - this.RangesManager = SandboxedModule.require(modulePath) + this.RangesManager = SandboxedModule.require(modulePath, { + requires: { './Metrics': (this.Metrics = { histogram: sinon.stub() }) }, + }) this.doc_id = 'doc-id-123' this.project_id = 'project-id-123' @@ -302,7 +304,7 @@ describe('RangesManager', function () { }) }) - return describe('with an update that collapses a range', function () { + describe('with an update that collapses a range', function () { beforeEach(function () { this.updates = [ { @@ -351,6 +353,70 @@ describe('RangesManager', function () { return expect(ranges_were_collapsed).to.equal(true) }) }) + + describe('with an update that deletes ranges', function () { + beforeEach(function () { + this.updates = [ + { + meta: { + user_id: this.user_id, + }, + op: [ + { + d: 'one two three four five', + p: 0, + }, + ], + }, + ] + this.entries = { + comments: [ + { + op: { + c: 'n', + p: 1, + t: 'thread-id-2', + }, + metadata: { + user_id: this.user_id, + }, + }, + ], + changes: [ + { + op: { + i: 'hello', + p: 1, + t: 'thread-id-2', + }, + metadata: { + user_id: this.user_id, + }, + }, + ], + } + return this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.entries, + this.updates, + this.newDocLines, + this.callback + ) + }) + + it('should increment the range-delta histogram', function () { + this.Metrics.histogram.called.should.equal(true) + }) + + return it('should return ranges_were_collapsed == true', function () { + this.callback.called.should.equal(true) + const [error, entries, ranges_were_collapsed] = Array.from( + this.callback.args[0] + ) + return expect(ranges_were_collapsed).to.equal(true) + }) + }) }) return describe('acceptChanges', function () { diff --git a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js index 2f23de7f68..21bb98bb0b 100644 --- a/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js +++ b/services/document-updater/test/unit/js/UpdateManager/UpdateManagerTests.js @@ -30,6 +30,7 @@ describe('UpdateManager', function () { './ShareJsUpdateManager': (this.ShareJsUpdateManager = {}), './HistoryManager': (this.HistoryManager = {}), './Metrics': (this.Metrics = { + inc: sinon.stub(), Timer: (Timer = (function () { Timer = class Timer { static initClass() { @@ -489,6 +490,10 @@ describe('UpdateManager', function () { ) }) + it('should increment the doc-snapshot metric', function () { + this.Metrics.inc.calledWith('doc-snapshot').should.equal(true) + }) + return it('should call SnapshotManager.recordSnapshot', function () { return this.SnapshotManager.recordSnapshot .calledWith(