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

Error stacktrace overhaul #1944

Merged
merged 14 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 2 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ lib/types/ResponsePayload.js
lib/types/StrategyDefinition.js
lib/types/Token.js
lib/types/User.js
lib/types/Global.d.ts
lib/types/Global.js
lib/util/interfaces.js
lib/util/mutex.js
features/support/application/functional-tests-app.js
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ lib/types/StrategyDefinition.js
lib/types/Token.js
lib/types/User.js
lib/types/index.js
lib/types/Global.d.ts
lib/types/Global.js
lib/util/interfaces.js
lib/util/mutex.js
features-sdk/support/application/functional-tests-app.js
2 changes: 1 addition & 1 deletion doc/2/guides/introduction/general-purpose-backend/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ The entire realtime engine is used exclusively from a client (frontend or backen

## Integrated Cluster Mode

Kuzzle is a backend designed to **scroll horizontally to millions of users**.
Kuzzle is a backend designed to **scale horizontally to millions of users**.

With its integrated [masterless cluster mode](/core/2/guides/advanced/cluster-scalability), it allows hot-starting new application instances to handle the load.

Expand Down
1 change: 0 additions & 1 deletion docker/scripts/start-kuzzle-dev.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import should from 'should'
import { omit } from 'lodash'
import Bluebird from 'bluebird';

import { Backend, Request, Mutex } from '../../index';
import { FunctionalTestsController } from './functional-tests-controller';
Expand Down
2 changes: 1 addition & 1 deletion lib/api/request/kuzzle-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ export class KuzzleRequest {
* @param message message displayed in the warning
*/
addDeprecation (version: string, message: string) {
if (process.env.NODE_ENV !== 'development') {
if (global.NODE_ENV !== 'development') {
return;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/core/network/protocols/mqtt.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,12 @@ class MqttProtocol extends Protocol {
_respond (client, response) {
debug('sending response: %o', response.content);

if (process.env.NODE_ENV === 'development' && this.config.developmentMode) {
if (global.NODE_ENV === 'development' && this.config.developmentMode) {
this.broadcast({
channels: [this.config.responseTopic],
payload: response.content,
});

return;
}

Expand Down
20 changes: 15 additions & 5 deletions lib/core/network/removeErrorStack.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@

const util = require('util');

const { hilightUserCode } = require('../../util/stackTrace');

/**
* utility method: must be invoked by all protocols to remove stack traces
* from payloads before sending them
Expand All @@ -31,12 +33,20 @@ const util = require('util');
* @returns {*} return the data minus the stack trace
*/
module.exports = data => {
if (process.env.NODE_ENV !== 'development') {
if (util.isError(data)) {
delete data.stack;
if (util.isError(data)) {
if (global.NODE_ENV !== 'development') {
data.stack = undefined;
}
else {
data.stack = data.stack.split('\n').map(hilightUserCode).join('\n');
}
}
else if (data && data.content && data.content.error) {
if (global.NODE_ENV !== 'development') {
data.content.error.stack = undefined;
}
else if (data && data.content && data.content.error) {
delete data.content.error.stack;
else {
data.content.error.stack = data.content.error.stack.split('\n').map(hilightUserCode).join('\n');
Copy link
Contributor

@Shiranuit Shiranuit Jan 29, 2021

Choose a reason for hiding this comment

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

      data.content.error.stack = data.content.error.stack.split('\n').map(hilightUserCode).join('\n');

Maybe having a function to do this would be better instead of repeating this bit of code everywhere ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't because I use this method in a custom way in the kerror/index.js file

}
}

Expand Down
12 changes: 3 additions & 9 deletions lib/core/shared/sdk/funnelProtocol.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const { KuzzleEventEmitter } = require('kuzzle-sdk');

const { Request } = require('../../../api/request');
const kerror = require('../../../kerror');
const { hilightUserCode } = require('../../../util/stackTrace');

class FunnelProtocol extends KuzzleEventEmitter {
constructor(user = null) {
Expand Down Expand Up @@ -57,12 +58,6 @@ class FunnelProtocol extends KuzzleEventEmitter {
* Hydrate the user and execute SDK query
*/
async query (request) {
// Keep a more complete stacktrace in development
let stack;
if (process.env.NODE_ENV !== 'production') {
stack = Error().stack;
}

if (! this.requestOptions) {
this.requestOptions = {
connection: {
Expand All @@ -81,9 +76,8 @@ class FunnelProtocol extends KuzzleEventEmitter {
return { result };
}
catch (error) {
// enhance stacktrace
if (process.env.NODE_ENV !== 'production') {
error.stack = stack;
if (error.stack) {
error.stack = error.stack.split('\n').map(hilightUserCode).join('\n');
}

throw error;
Expand Down
19 changes: 11 additions & 8 deletions lib/kerror/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const _ = require('lodash');

const { domains } = require('./codes');
const errors = require('./errors');
const { hilightUserCode } = require('../util/stackTrace');

/**
* Gets this file name in the exact same format than the one printed in the
Expand Down Expand Up @@ -113,17 +114,19 @@ function cleanStackTrace (error) {
// we keep the new error instantiation line ("new ...Error (") on purpose:
// this will allow us to replace it without inserting a new line in the array,
// saving us from building a new array
const newStack = error.stack.split('\n').filter((line, index) => {
if (index < messageLength) {
return true;
}
const newStack = error.stack.split('\n')
.filter((line, index) => {
if (index < messageLength) {
return true;
}

// filter all lines related to the kerror object
return !line.includes(currentFileName);
});
// filter all lines related to the kerror object
return !line.includes(currentFileName);
})
.map(hilightUserCode);

// insert a deletion message in place of the new error instantiation line
newStack[messageLength] = ' [...Kuzzle internal calls deleted...]';
newStack[messageLength] = ' [...Kuzzle internal calls deleted...]';

error.stack = newStack.join('\n');
}
Expand Down
4 changes: 2 additions & 2 deletions lib/kuzzle/kuzzle.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,9 @@ class Kuzzle extends KuzzleEventEmitter {
// Crashing on an unhandled rejection is a good idea during development
// as it helps spotting code errors. And according to the warning messages,
// this is what Node.js will do automatically in future versions anyway.
if (process.env.NODE_ENV === 'development') {
if (global.NODE_ENV === 'development') {
this.log.error('Kuzzle caught an unhandled rejected promise and will shutdown.');
this.log.error('This behavior is only triggered if NODE_ENV is set to "development"');
this.log.error('This behavior is only triggered if global.NODE_ENV is set to "development"');

throw reason;
}
Expand Down
4 changes: 2 additions & 2 deletions lib/service/storage/elasticsearch.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class ElasticSearch extends Service {
return;
}

if ( process.env.NODE_ENV === 'production'
if ( global.NODE_ENV === 'production'
&& this._config.commonMapping.dynamic === 'true'
) {
global.kuzzle.log.warn([
Expand Down Expand Up @@ -845,7 +845,7 @@ class ElasticSearch extends Service {
id,
index: esIndex,
};

try {
debug('DeleteFields document: %o', esRequest);
const { body } = await this._client.get(esRequest);
Expand Down
2 changes: 1 addition & 1 deletion lib/service/storage/esWrapper.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ class ESWrapper {
return error;
}

if (process.env.NODE_ENV === 'production') {
if (global.NODE_ENV === 'production') {
global.kuzzle.log.info(JSON.stringify({
message: `Elasticsearch Client error: ${error.message}`,
// /!\ not all ES error classes have a "meta" property
Expand Down
2 changes: 1 addition & 1 deletion lib/types/Deprecation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

/**
* Deprecation warning about a specific feature.
* Only available in developement mode (NODE_ENV=development)
* Only available in developement mode (global.NODE_ENV=development)
*/
export type Deprecation = {
/**
Expand Down
9 changes: 0 additions & 9 deletions lib/types/Global.d.ts

This file was deleted.

16 changes: 16 additions & 0 deletions lib/types/Global.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/* eslint-disable @typescript-eslint/no-namespace */

declare global {
namespace NodeJS {
interface Global {
kuzzle: any;
NODE_ENV: string;
}
}
}

global.NODE_ENV = process.env.NODE_ENV;

export {};

/* eslint-enable @typescript-eslint/no-namespace */
2 changes: 1 addition & 1 deletion lib/types/ResponsePayload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export type ResponseErrorPayload = {
status: number;

/**
* Error stacktrace (only if NODE_ENV=development)
* Error stacktrace (only if global.NODE_ENV=development)
*/
stack?: string;
};
Expand Down
1 change: 1 addition & 0 deletions lib/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,4 @@ export * from './EventHandler';
export * from './User';
export * from './Token';
export * from './InternalLogger';
export * from './Global';
4 changes: 2 additions & 2 deletions lib/util/deprecate.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
'use strict';

const deprecationWarning = (logger, ...args) => {
if (process.env.NODE_ENV !== 'production') {
if (global.NODE_ENV !== 'production') {
logger.warn('DEPRECATION WARNING');
logger.warn(...args);
}
Expand All @@ -49,7 +49,7 @@ const warnIfDeprecated = (logger, deprecations, member) => {
};

const deprecateProperties = (logger, target, deprecations = {}) => {
if (process.env.NODE_ENV === 'production') {
if (global.NODE_ENV === 'production') {
return target;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/util/didYouMean.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
const didYouMean = require('didyoumean');

function printDidYouMean(...args) {
if (process.env.NODE_ENV === 'production') {
if (global.NODE_ENV === 'production') {
return '';
}

Expand Down
53 changes: 53 additions & 0 deletions lib/util/stackTrace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Kuzzle, a backend software, self-hostable and ready to use
* to power modern apps
*
* Copyright 2015-2020 Kuzzle
* mailto: support AT kuzzle.io
* website: http://kuzzle.io
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

'use strict';

/**
* Hilight user code
*
* e.g.
* at BackendController._add (/home/kuzzle/lib/core/application/backend.ts:261:28)
* at BackendController.register (/home/kuzzle/lib/core/application/backend.ts:187:10)
* > at registerFoo (/home/aschen/projets/app/test.ts:12:18)
* > at init (/home/aschen/projets/app/test.ts:8:3)
* at Module._compile (internal/modules/cjs/loader.js:1133:30)
*/
function hilightUserCode (line) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong but, rather than highlighting user code, this function filters out system code. #renaming #nitpicking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nop, it does not remove any line from the stacktrace

// ignore first line (error message) or already enhanced
if (! line.includes(' at ') || line.startsWith('>')) {
return line;
}

if ( line.includes('kuzzle/lib/') // ignore kuzzle code
|| (line.indexOf('at /') === -1 && line.charAt(line.indexOf('(') + 1) !== '/') // ignore node internal
|| line.includes('node_modules') // ignore dependencies
) {
return ' ' + line;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we include the line if the comments say it is ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it's an internal line from Node.js or from Kuzzle so we keep it either way because it's a bad pratice to hide stack calls (for us it's more difficult to debug Kuzzle bug, for users it can lead to confusion)

}

// hilight user code
return '>' + line;
}

module.exports = {
hilightUserCode,
};
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"koncorde": "^3.0.0",
"kuzzle-plugin-auth-passport-local": "^6.2.0",
"kuzzle-plugin-logger": "^2.0.7",
"kuzzle-sdk": "^7.4.9",
"kuzzle-sdk": "^7.5.2",
"kuzzle-vault": "^2.0.0",
"lodash": "4.17.20",
"moment": "^2.29.1",
Expand Down
4 changes: 2 additions & 2 deletions plugins/available/kuzzle-plugin-cluster/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -839,10 +839,10 @@ class KuzzleCluster {
// Crashing on an unhandled rejection is a good idea during development
// as it helps spotting code errors. And according to the warning messages,
// this is what Node.js will do automatically in future versions anyway.
if (process.env.NODE_ENV === 'development') {
if (global.NODE_ENV === 'development') {
process.on('unhandledRejection', () => {
this.log('error', 'Kuzzle caught an unhandled rejected promise and will shutdown.');
this.log('error', 'This behavior is only triggered if NODE_ENV is set to "development"');
this.log('error', 'This behavior is only triggered if global.NODE_ENV is set to "development"');
this._onShutDown('unhandledRejection');
});
}
Expand Down
Loading