Clean up error reporting
Change-Id: I63bc4c0f6e2aa4c6ebae6be85860be3b422d6615
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 020762c..1a8f80f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1150,7 +1150,7 @@
revert dialog after revert button is pressed. */
this.restApiService.getChanges(0, query).then(changes => {
if (!changes) {
- console.error('changes is undefined');
+ this.reporting.error(new Error('changes is undefined'));
return;
}
this.$.confirmRevertDialog.populate(change, this.commitMessage, changes);
@@ -1164,7 +1164,7 @@
const query = `submissionid:${change.submission_id}`;
this.restApiService.getChanges(0, query).then(changes => {
if (!changes) {
- console.error('changes is undefined');
+ this.reporting.error(new Error('changes is undefined'));
return;
}
this.$.confirmRevertSubmissionDialog._populateRevertSubmissionMessage(
@@ -1442,7 +1442,7 @@
);
break;
default:
- console.error('invalid revert type');
+ this.reporting.error(new Error('invalid revert type'));
}
}
@@ -1794,7 +1794,7 @@
.getChanges(0, query, undefined, options)
.then(changes => {
if (!changes) {
- console.error('getChanges returns undefined');
+ this.reporting.error(new Error('getChanges returns undefined'));
return;
}
this.$.confirmCherrypick.updateChanges(changes);
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index 6a4d5b1..51fc5b5 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -360,7 +360,7 @@
input: string
): Promise<AutocompleteSuggestion[]> {
if (!this.project) {
- console.error('no project specified');
+ this.reporting.error(new Error('no project specified'));
return Promise.resolve([]);
}
if (input.startsWith('refs/heads/')) {
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index d0d0745..c3ee8e5 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -561,7 +561,7 @@
for (const splice of indexSplices) {
for (const account of splice.removed) {
if (!this._reviewersPendingRemove[type]) {
- console.error('Invalid type ' + type + ' for reviewer.');
+ this.reporting.error(new Error(`Invalid type ${type} for reviewer.`));
return;
}
this._reviewersPendingRemove[type].push(account);
@@ -1120,8 +1120,11 @@
if (containsAll(currentAttentionSet, newAttentionSet)) {
return 'No additions to the attention set.';
}
- console.error(
- '_computeDoNotUpdateMessage() should not be called when users were added to the attention set.'
+ this.reporting.error(
+ new Error(
+ '_computeDoNotUpdateMessage()' +
+ 'should not be called when users were added to the attention set.'
+ )
);
return '';
}
@@ -1318,7 +1321,9 @@
this._focusOn(FocusTarget.REVIEWERS);
return;
}
- console.error('_confirmPendingReviewer called without pending confirm');
+ this.reporting.error(
+ new Error('_confirmPendingReviewer called without pending confirm')
+ );
}
_cancelPendingReviewer() {
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index a1909c8..b39481c 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -790,7 +790,9 @@
authRedirect?: boolean
) {
if (!this[handlerName]) {
- console.error('Attempted to map route to unknown method: ', handlerName);
+ this.reporting.error(
+ new Error(`Attempted to map route to unknown method: ${handlerName}`)
+ );
return;
}
page(
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index 83840a5f..b01ac51 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -125,6 +125,8 @@
private readonly restApiService = appContext.restApiService;
+ reporting = appContext.reportingService;
+
get keyBindings() {
return {
'ctrl+s meta+s': '_handleSaveShortcut',
@@ -330,7 +332,7 @@
this._saveEdit().then(() => {
const handleError: ErrorCallback = response => {
this._showAlert(PUBLISH_FAILED_MSG);
- console.error(response);
+ this.reporting.error(new Error(response?.statusText));
};
this._showAlert(PUBLISHING_EDIT_MSG);
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
index 0cb628c..a493e51 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
@@ -18,6 +18,7 @@
import {GrAnnotation} from '../../diff/gr-diff-highlight/gr-annotation';
import {GrStyleObject} from '../../plugins/gr-styles-api/gr-styles-api';
import {GrDiffLine} from '../../diff/gr-diff/gr-diff-line';
+import {appContext} from '../../../services/app-context';
/**
* Used to create a context for GrAnnotationActionsInterface.
@@ -42,6 +43,8 @@
changeNum: number;
+ private readonly reporting = appContext.reportingService;
+
constructor(
contentEl: HTMLElement,
lineNumberEl: HTMLElement,
@@ -56,8 +59,8 @@
this.path = path;
this.changeNum = Number(changeNum);
if (isNaN(this.changeNum)) {
- console.error(
- `GrAnnotationActionsContext: Invalid changeNum: ${changeNum}`
+ this.reporting.error(
+ new Error(`GrAnnotationActionsContext: Invalid changeNum: ${changeNum}`)
);
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
index e069f8b..f160807 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
@@ -24,6 +24,7 @@
import {Side} from '../../../constants/constants';
import {PluginApi} from '../../plugins/gr-plugin-types';
import {ChangeInfo, NumericChangeId} from '../../../types/common';
+import {appContext} from '../../../services/app-context';
type AddLayerFunc = (ctx: GrAnnotationActionsContext) => void;
@@ -52,6 +53,8 @@
// Default impl is a no-op.
private addLayerFunc: AddLayerFunc = () => {};
+ reporting = appContext.reportingService;
+
constructor(private readonly plugin: PluginApi) {
// Return this instance when there is an annotatediff event.
plugin.on('annotatediff', this);
@@ -134,12 +137,14 @@
) {
this.plugin.hook('annotation-toggler').onAttached(element => {
if (!element.content) {
- console.error('plugin endpoint without content.');
+ this.reporting.error(new Error('plugin endpoint without content.'));
return;
}
if (!element.content.hidden) {
- console.error(
- element.content.id + ' is already enabled. Cannot re-enable.'
+ this.reporting.error(
+ new Error(
+ `${element.content.id} is already enabled. Cannot re-enable.`
+ )
);
return;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
index 8b3f501..7ae34cf 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
@@ -133,12 +133,9 @@
// Assert that error is shown if we try to enable checkbox again.
onAttachedFuncCalled = false;
annotationActions.enableToggleCheckbox('test label2', onAttachedFunc);
- const errorStub = sinon.stub(
- console, 'error').callsFake((msg, err) => undefined);
+ const errorStub = sinon.stub(annotationActions.reporting, 'error');
emulateAttached();
- assert.isTrue(
- errorStub.calledWith(
- 'annotation-span is already enabled. Cannot re-enable.'));
+ assert.isTrue(errorStub.called);
// Assert that onAttachedFunc is not called and the label has not changed.
assert.isFalse(onAttachedFuncCalled);
assert.equal(document.getElementById('annotation-label').textContent,
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index 1bcf190..ae6e155 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -36,6 +36,7 @@
import {EventType, TargetElement} from '../../plugins/gr-plugin-types';
import {DiffLayer, HighlightJS} from '../../../types/types';
import {ParsedChangeInfo} from '../gr-rest-api-interface/gr-reviewer-updates-parser';
+import {appContext} from '../../../services/app-context';
const elements: {[key: string]: HTMLElement} = {};
const eventCallbacks: {[key: string]: EventCallback[]} = {};
@@ -44,6 +45,8 @@
export class GrJsApiInterface
extends GestureEventListeners(LegacyElementMixin(PolymerElement))
implements JsApiService {
+ private readonly reporting = appContext.reportingService;
+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
handleEvent(type: EventType, detail: any) {
getPluginLoader()
@@ -99,7 +102,7 @@
try {
return callback(change, revision) === false;
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
return false;
});
@@ -120,7 +123,7 @@
try {
cb(detail.path);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -161,7 +164,7 @@
try {
cb(change, revision, info);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -174,7 +177,7 @@
try {
cb(detail.revisionActions, detail.change);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -184,7 +187,7 @@
try {
cb(change, msg);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -195,7 +198,7 @@
try {
cb(detail.node);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -205,7 +208,7 @@
try {
cb(detail.change);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -215,7 +218,7 @@
try {
cb(detail.hljs);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -225,7 +228,7 @@
try {
revertMsg = cb(change, revertMsg, origMsg) as string;
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
return revertMsg;
@@ -244,7 +247,7 @@
origMsg
) as string;
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
return revertSubmissionMsg;
@@ -258,7 +261,7 @@
const layer = annotationApi.getLayer(path, changeNum);
layers.push(layer);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
return layers;
@@ -270,7 +273,7 @@
const annotationApi = (cb as unknown) as GrAnnotationActionsInterface;
annotationApi.disposeLayer(path);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
}
@@ -311,7 +314,7 @@
try {
labels = cb(change);
} catch (err) {
- console.error(err);
+ this.reporting.error(err);
}
}
return labels;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
index b5c4e48..1481c2d 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
@@ -24,6 +24,7 @@
import {getPluginLoader} from './gr-plugin-loader.js';
import {_testOnly_initGerritPluginApi} from './gr-gerrit.js';
import {stubBaseUrl} from '../../../test/test-utils.js';
+import sinon from 'sinon/pkg/sinon-esm';
const basicFixture = fixtureFromElement('gr-js-api-interface');
@@ -57,7 +58,7 @@
},
});
element = basicFixture.instantiate();
- errorStub = sinon.stub(console, 'error');
+ errorStub = sinon.stub(element.reporting, 'error');
pluginApi.install(p => { plugin = p; }, '0.1',
'http://test.com/plugins/testplugin/static/test.js');
getPluginLoader().loadPlugins([]);
@@ -404,6 +405,7 @@
suite('popup', () => {
test('popup(element) is deprecated', () => {
+ sinon.stub(console, 'error');
plugin.popup(document.createElement('div'));
assert.isTrue(console.error.calledOnce);
});
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
index c47f3e2..48c90f7 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
@@ -55,6 +55,7 @@
reportExtension(name: string): void;
pluginLoaded(name: string): void;
pluginsLoaded(pluginsList?: string[]): void;
+ error(err: unknown, reporter?: string, details?: EventDetails): void;
/**
* Reset named timer.
*/
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index 668834e..b5c0d6d 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -114,7 +114,22 @@
export function initErrorReporter(appContext: AppContext) {
const reportingService = appContext.reportingService;
- // TODO(dmfilippo): TS-fix-any oldOnError - define correct type
+
+ const normalizeError = (err: Error | unknown) => {
+ if (err instanceof Error) {
+ return err;
+ }
+ let msg = '';
+ if (typeof err === 'string') {
+ msg += err;
+ } else {
+ msg += JSON.stringify(err);
+ }
+ const error = new Error(msg);
+ error.stack = 'unknown';
+ return error;
+ };
+ // TODO(dmfilippov): TS-fix-any oldOnError - define correct type
const onError = function (
oldOnError: Function,
msg: Event | string,
@@ -127,29 +142,15 @@
oldOnError(msg, url, line, column, error);
}
if (error) {
- line = line || error.lineNumber;
- column = column || error.columnNumber;
- let shortenedErrorStack = msg;
- if (error.stack) {
- const errorStackLines = error.stack.split('\n');
- shortenedErrorStack = errorStackLines
- .slice(0, Math.min(3, errorStackLines.length))
- .join('\n');
- }
- msg = shortenedErrorStack || error.toString();
+ line = line ?? error.lineNumber;
+ column = column ?? error.columnNumber;
}
- const payload = {
- url,
+ reportingService.error(normalizeError(error), 'onError', {
line,
column,
- error,
- };
- reportingService.reporter(
- ERROR.TYPE,
- ERROR.CATEGORY.EXCEPTION,
- `${msg}`,
- payload
- );
+ url,
+ msg,
+ });
return true;
};
// TODO(dmfilippov): TS-fix-any unclear what is context
@@ -169,16 +170,7 @@
context.addEventListener(
'unhandledrejection',
(e: PromiseRejectionEvent) => {
- const msg = e.reason.message;
- const payload = {
- error: e.reason,
- };
- reportingService.reporter(
- ERROR.TYPE,
- ERROR.CATEGORY.EXCEPTION,
- msg,
- payload
- );
+ reportingService.error(normalizeError(e.reason), 'unhandledrejection');
}
);
};
@@ -809,6 +801,19 @@
timer.end().reset();
}
+ error(error: Error, errorSource?: string, details?: EventDetails) {
+ const eventDetails = details ?? {};
+ const message = `${errorSource ? errorSource + ': ' : ''}${error.message}`;
+
+ this.reporter(
+ ERROR.TYPE,
+ ERROR.CATEGORY.EXCEPTION,
+ message,
+ {error},
+ {...eventDetails, stack: error.stack}
+ );
+ }
+
reportErrorDialog(message: string) {
this.reporter(
ERROR.TYPE,
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
index 924ddd9..6f75f29 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
@@ -50,6 +50,7 @@
recordDraftInteraction: () => {},
reporter: () => {},
reportErrorDialog: () => {},
+ error: () => {},
reportExtension: () => {},
reportInteraction: () => {},
reportLifeCycle: () => {},
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_test.js b/polygerrit-ui/app/services/gr-reporting/gr-reporting_test.js
index 08e4a55..6e56ab1 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_test.js
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_test.js
@@ -463,23 +463,14 @@
const error = new Error('bar');
error.stack = undefined;
emulateThrow('bar', 'http://url', 4, 2, error);
- assert.isTrue(reporter.calledWith('error', 'exception', 'bar'));
- const payload = reporter.lastCall.args[3];
- assert.deepEqual(payload, {
- url: 'http://url',
- line: 4,
- column: 2,
- error,
- });
+ assert.isTrue(reporter.calledWith('error', 'exception', 'onError: bar'));
});
- test('is reported with 3 lines of stack', () => {
+ test('is reported with stack', () => {
const error = new Error('bar');
emulateThrow('bar', 'http://url', 4, 2, error);
- const expectedStack = error.stack.split('\n').slice(0, 3)
- .join('\n');
- assert.isTrue(reporter.calledWith('error', 'exception',
- expectedStack));
+ const eventDetails = reporter.lastCall.args[4];
+ assert.equal(error.stack, eventDetails.stack);
});
test('prevent default event handler', () => {
@@ -487,12 +478,10 @@
});
test('unhandled rejection', () => {
- fakeWindow.handlers['unhandledrejection']({
- reason: {
- message: 'bar',
- },
- });
- assert.isTrue(reporter.calledWith('error', 'exception', 'bar'));
+ const newError = new Error('bar');
+ fakeWindow.handlers['unhandledrejection']({reason: newError});
+ assert.isTrue(reporter.calledWith('error', 'exception',
+ 'unhandledrejection: bar'));
});
});
});
