(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
This commit is contained in:
Alex Hall 2022-07-15 00:19:18 +02:00
parent 333ed863f4
commit 1a6e1872de
4 changed files with 14 additions and 10 deletions

View File

@ -644,6 +644,8 @@ export class ActiveDoc extends EventEmitter {
}); });
}); });
} catch (err) { } catch (err) {
const level = err.status === 404 ? "warn" : "error";
this._log.log(level, docSession, "Failed to load document", err);
await this.shutdown(); await this.shutdown();
throw err; throw err;
} }

View File

@ -456,7 +456,8 @@ export class Client {
(typeof code === 'string' && code.startsWith('AUTH_NO')) (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}; response = {reqId: request.reqId, error: err.message};
if (err.code) { if (err.code) {
response.errorCode = err.code; response.errorCode = err.code;

View File

@ -1,4 +1,5 @@
import * as sqlite3 from '@gristlabs/sqlite3'; import * as sqlite3 from '@gristlabs/sqlite3';
import {ApiError} from 'app/common/ApiError';
import {mapGetOrSet} from 'app/common/AsyncCreate'; import {mapGetOrSet} from 'app/common/AsyncCreate';
import {delay} from 'app/common/delay'; import {delay} from 'app/common/delay';
import {DocEntry} from 'app/common/DocListAPI'; import {DocEntry} from 'app/common/DocListAPI';
@ -596,9 +597,9 @@ export class HostedStorageManager implements IDocStorageManager {
// skip S3, just use file system // skip S3, just use file system
let present: boolean = await fse.pathExists(this.getPath(docName)); let present: boolean = await fse.pathExists(this.getPath(docName));
if ((forkId || snapshotId) && !present) { 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') { 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))) { if (await fse.pathExists(this.getPath(trunkId))) {
await fse.copy(this.getPath(trunkId), this.getPath(docName)); 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 (!await this._ext.exists(destIdWithoutSnapshot)) {
if (!options.trunkId) { return false; } // Document not found in S3 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). // 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. // The special NEW_DOCUMENT_CODE trunk means we should create an empty document.
if (options.trunkId === NEW_DOCUMENT_CODE) { return false; } 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; sourceDocId = options.trunkId;
} }
await this._ext.downloadTo(sourceDocId, destId, this.getPath(destId), options.snapshotId); await this._ext.downloadTo(sourceDocId, destId, this.getPath(destId), options.snapshotId);

View File

@ -330,12 +330,12 @@ describe("Fork", function() {
await altSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); await altSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false);
assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); 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 // Ensure the user has a way to report the problem (although including the report button in
// the modal might be better). // the modal might be better).
assert.match(await driver.find('.test-notifier-toast-wrapper').getText(), 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 // A new doc cannot be created either (because of access
// mismatch - for forks of the doc used in these tests, user2 // mismatch - for forks of the doc used in these tests, user2
@ -350,7 +350,7 @@ describe("Fork", function() {
const anonSession = await altSession.anon.login(); const anonSession = await altSession.anon.login();
await anonSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); await anonSession.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false);
assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); 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). // A new doc cannot be created either (because of access mismatch).
await altSession.loadDoc(`/doc/new~${forkId}~${userId}`, false); await altSession.loadDoc(`/doc/new~${forkId}~${userId}`, false);
@ -362,12 +362,12 @@ describe("Fork", function() {
await team.login(); await team.login();
await team.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false); await team.loadDoc(`/doc/${doc.id}~${forkId}~${userId}`, false);
assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); 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. // New document can no longer be casually created this way anymore either.
await team.loadDoc(`/doc/new~${forkId}~${userId}`, false); await team.loadDoc(`/doc/new~${forkId}~${userId}`, false);
assert.equal(await driver.findWait('.test-modal-dialog', 2000).isDisplayed(), true); 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(); await gu.wipeToasts();
}); });