From 1a6e1872dee8a72013321b651ab12ed9a6aebb48 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Fri, 15 Jul 2022 00:19:18 +0200 Subject: [PATCH] (core) Miscellaneous little logging improvements Summary: 1. Log errors in `ActiveDoc.loadDoc` as errors, not just warnings, except for a common 'Cannot create fork' error caused by deployment tests. 2. Log the method name that had an error in `server/lib/Client.ts`. Discussion: https://grist.slack.com/archives/CR8HZ4P9V/p1652364998893169 Following up on https://phab.getgrist.com/D3522 Test Plan: tested manually, particularly by running the nbrowser/Fork test that led to the initial noisy errors in Slack. Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D3525 --- app/server/lib/ActiveDoc.ts | 2 ++ app/server/lib/Client.ts | 3 ++- app/server/lib/HostedStorageManager.ts | 9 +++++---- test/nbrowser/Fork.ts | 10 +++++----- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 96a79f52..399bfdef 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -644,6 +644,8 @@ export class ActiveDoc extends EventEmitter { }); }); } catch (err) { + const level = err.status === 404 ? "warn" : "error"; + this._log.log(level, docSession, "Failed to load document", err); await this.shutdown(); throw err; } diff --git a/app/server/lib/Client.ts b/app/server/lib/Client.ts index 73bb9a6f..c2221a69 100644 --- a/app/server/lib/Client.ts +++ b/app/server/lib/Client.ts @@ -456,7 +456,8 @@ export class Client { (typeof code === 'string' && code.startsWith('AUTH_NO')) ); - this._log.warn(null, "Error %s %s", skipStack ? err : err.stack, code || ''); + this._log.warn(null, "Responding to method %s with error: %s %s", + request.method, skipStack ? err : err.stack, code || ''); response = {reqId: request.reqId, error: err.message}; if (err.code) { response.errorCode = err.code; diff --git a/app/server/lib/HostedStorageManager.ts b/app/server/lib/HostedStorageManager.ts index fded820e..ac33b4e1 100644 --- a/app/server/lib/HostedStorageManager.ts +++ b/app/server/lib/HostedStorageManager.ts @@ -1,4 +1,5 @@ import * as sqlite3 from '@gristlabs/sqlite3'; +import {ApiError} from 'app/common/ApiError'; import {mapGetOrSet} from 'app/common/AsyncCreate'; import {delay} from 'app/common/delay'; import {DocEntry} from 'app/common/DocListAPI'; @@ -596,9 +597,9 @@ export class HostedStorageManager implements IDocStorageManager { // skip S3, just use file system let present: boolean = await fse.pathExists(this.getPath(docName)); if ((forkId || snapshotId) && !present) { - if (!canCreateFork) { throw new Error(`Cannot create fork`); } + if (!canCreateFork) { throw new ApiError("Document fork not found", 404); } if (snapshotId && snapshotId !== 'current') { - throw new Error(`cannot find snapshot ${snapshotId} of ${docName}`); + throw new ApiError(`cannot find snapshot ${snapshotId} of ${docName}`, 404); } if (await fse.pathExists(this.getPath(trunkId))) { await fse.copy(this.getPath(trunkId), this.getPath(docName)); @@ -681,10 +682,10 @@ export class HostedStorageManager implements IDocStorageManager { if (!await this._ext.exists(destIdWithoutSnapshot)) { if (!options.trunkId) { return false; } // Document not found in S3 // No such fork in s3 yet, try from trunk (if we are allowed to create the fork). - if (!options.canCreateFork) { throw new Error('Cannot create fork'); } + if (!options.canCreateFork) { throw new ApiError("Document fork not found", 404); } // The special NEW_DOCUMENT_CODE trunk means we should create an empty document. if (options.trunkId === NEW_DOCUMENT_CODE) { return false; } - if (!await this._ext.exists(options.trunkId)) { throw new Error('Cannot find original'); } + if (!await this._ext.exists(options.trunkId)) { throw new ApiError('Cannot find original', 404); } sourceDocId = options.trunkId; } await this._ext.downloadTo(sourceDocId, destId, this.getPath(destId), options.snapshotId); diff --git a/test/nbrowser/Fork.ts b/test/nbrowser/Fork.ts index 40db59dd..ceb9e8f2 100644 --- a/test/nbrowser/Fork.ts +++ b/test/nbrowser/Fork.ts @@ -330,12 +330,12 @@ describe("Fork", function() { await altSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); - assert.match(await driver.find('.test-modal-dialog').getText(), /Cannot create fork/); + assert.match(await driver.find('.test-modal-dialog').getText(), /Document fork not found/); // Ensure the user has a way to report the problem (although including the report button in // the modal might be better). assert.match(await driver.find('.test-notifier-toast-wrapper').getText(), - /Cannot create fork.*Report a problem/s); + /Document fork not found.*Report a problem/s); // A new doc cannot be created either (because of access // mismatch - for forks of the doc used in these tests, user2 @@ -350,7 +350,7 @@ describe("Fork", function() { const anonSession = await altSession.anon.login(); await anonSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); - assert.match(await driver.find('.test-modal-dialog').getText(), /Cannot create fork/); + assert.match(await driver.find('.test-modal-dialog').getText(), /Document fork not found/); // A new doc cannot be created either (because of access mismatch). await altSession.loadDoc(`/doc/new~${forkId}~${userId}`, false); @@ -362,12 +362,12 @@ describe("Fork", function() { await team.login(); await team.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); - assert.match(await driver.find('.test-modal-dialog').getText(), /Cannot create fork/); + assert.match(await driver.find('.test-modal-dialog').getText(), /Document fork not found/); // New document can no longer be casually created this way anymore either. await team.loadDoc(`/doc/new~${forkId}~${userId}`, false); assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); - assert.match(await driver.find('.test-modal-dialog').getText(), /Cannot create fork/); + assert.match(await driver.find('.test-modal-dialog').getText(), /Document fork not found/); await gu.wipeToasts(); });