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.

Sign up for GitHub

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

New: Log preview_error message from Preview #619

Closed
wants to merge 12 commits into from
Closed

New: Log preview_error message from Preview #619

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
28c1cc2
New: Log preview_error message from Preview
Jan 31, 2018
4fa127c
Chore: Docs for ERROR_CODE
Jan 31, 2018
f8d7960
Chore: Strip auth from message and displayMessage
Jan 31, 2018
5bffa55
Chore: use .code for error code. Extracted util methods
Feb 2, 2018
d8e7a3f
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Feb 2, 2018
7464a4a
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Feb 6, 2018
f9463c5
Chore: tests for logPreviewError, createLog
Feb 6, 2018
5a299f1
Chore: update and added new tests
Feb 7, 2018
73cb72a
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Feb 7, 2018
6236dc5
Chore: empty case for error event. safety check before token strip
Feb 7, 2018
0394df8
Chore: Removed debug statement
Feb 7, 2018
82c4e0a
Merge branch 'master' of github.com:JustinHoldstock/box-content-previ…
Feb 7, 2018
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
45 changes: 43 additions & 2 deletions src/lib/Preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ import {
X_REP_HINT_VIDEO_DASH,
X_REP_HINT_VIDEO_MP4
} from './constants';
import { VIEWER_EVENT } from './events';
import { VIEWER_EVENT, ERROR_CODE } from './events';
import './Preview.scss';

const DEFAULT_DISABLED_VIEWERS = ['Office']; // viewers disabled by default
Expand Down Expand Up @@ -329,6 +329,10 @@ class Preview extends EventEmitter {
/* eslint-disable no-console */
console.error('[Preview SDK] Tried to cache invalid file: ', file);
/* eslint-enable no-console */

const err = new Error(ERROR_CODE.invalidCacheAttempt);
err.data = file;
this.logPreviewError(err);
}
});
}
Expand Down Expand Up @@ -531,6 +535,11 @@ class Preview extends EventEmitter {
/* eslint-disable no-console */
console.error(`Error prefetching file ID ${fileId} - ${err}`);
/* eslint-enable no-console */

const error = new Error(ERROR_CODE.prefetchFileId);
Copy link
Contributor Author

@JustinHoldstock JustinHoldstock Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I should abstract away, this functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

@JustinHoldstock Maybe it would make sense to make a PreviewError which extends Error

error.data = fileId;
this.logPreviewError(error);

return;
}

Expand Down Expand Up @@ -992,6 +1001,9 @@ class Preview extends EventEmitter {
case VIEWER_EVENT.mediaEndAutoplay:
this.navigateRight();
break;
case VIEWER_EVENT.error:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this case entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot. This prevents a double error message from being emitted, which would double up on firing off preview_error events.

this.logPreviewError(data.data);
break;
default:
// This includes 'notification', 'preload' and others
this.emit(data.event, data.data);
Expand Down Expand Up @@ -1152,12 +1164,16 @@ class Preview extends EventEmitter {

// Check if hit the retry limit
if (this.retryCount > RETRY_COUNT) {
let errorCode = ERROR_CODE.retriesExceeded;
let errorMessage = __('error_refresh');
if (err.response && err.response.status === 429) {
errorCode = ERROR_CODE.rateLimit;
errorMessage = __('error_rate_limit');
}

this.triggerError(new Error(errorMessage));
const error = new Error(errorCode);
error.displayMessage = errorMessage;
this.triggerError(error);
return;
}

Expand Down Expand Up @@ -1202,6 +1218,9 @@ class Preview extends EventEmitter {
* @return {void}
*/
triggerError(err) {
// Always log preview errors
this.logPreviewError(err);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a chance of double logging here. Above you bound to the VIEWER_EVENT.error event, which you then call to triggerError. On line 966 of your patch (this is old code), we also bind to the error event coming from the viewer and call triggerError. This would end up calling logPreviewError twice, one from handleViewerEvents and one from triggerError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aww hell. So, as it is, node requires that you bind to the 'error' event (listener #1), then we listen for it in handleViewerEvents (listener #2). The huge issue here is when #1 is bound can be after any setup errors occur.

Does it make sense that we begin listening for errors immediately on viewer creation, and not in handleViewerEvents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've confirmed that handleViewerEvents() only handles the generic viewerevent, therefore our initial thoughts are invalid. Merging


// If preview is closed don't do anything
if (!this.open) {
return;
Expand All @@ -1226,6 +1245,22 @@ class Preview extends EventEmitter {
this.viewer.load(err);
}

/**
* Message, to any listeners of Preview, that an error has occurred.
*
* @private
* @param {Error} error - The error that occurred.
* @return {void}
*/
logPreviewError(error) {
const errorMessage = {
error,
data: error.data || ''
Copy link
Contributor

Choose a reason for hiding this comment

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

How would we access the error type for analytics/debugging? Would that be in the error.message? Would that potentially overwrite a message we would get such as cannot access property hasTouch of undefined?

Copy link
Contributor Author

@JustinHoldstock JustinHoldstock Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jeremypress

For logging purposes, we specify our own error codes (see events.js). If we have not thrown the error ourselves, setting the message to one of our error codes, the error.message will be that of what the browser created.

I'm thinking a single logged event will be as follows:

{
  event_type: <error | metric>
  logger_session_id: <string> // ID of session 
  timestamp: <string>, // ISO Format
  file_id: <string>,
  file_version_id: <string>,
  content_type: <string>, // Content type of the file previewed
  extension: <string>,
  client_version: <string>, // preview version number
  browser_name: <string>,
  locale: <string>,                        
  event_name: <string>, // Either our error code OR browser error message
  value: <any> // Must be serializable
}

Note: I just realized I need to update so that we're sending more session related info, per prevew_error event.

Copy link
Contributor

@jeremypress jeremypress Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should we separate that out so that we have
error.code: our custom code we made or a generic one if the browser threw it
error.message: what the browser created / more info from if required

Second part sounds good

};

this.emit('preview_error', errorMessage);
}

/**
* Builds a list of required XHR headers.
*
Expand Down Expand Up @@ -1303,13 +1338,19 @@ class Preview extends EventEmitter {
/* eslint-disable no-console */
console.error(`Error prefetching file ID ${id} - ${err}`);
/* eslint-enable no-console */
const error = new Error(ERROR_CODE.prefetchFileId);
error.data = id;
this.logPreviewError(error);
});
});
})
.catch(() => {
/* eslint-disable no-console */
console.error('Error prefetching files');
/* eslint-enable no-console */
const error = new Error(ERROR_CODE.prefetchFileId);
error.data = filesToPrefetch;
this.logPreviewError(error);
});
}

Expand Down
19 changes: 12 additions & 7 deletions src/lib/RepStatus.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,26 +106,31 @@ class RepStatus extends EventEmitter {
*/
handleResponse() {
const status = RepStatus.getStatus(this.representation);
let errorCode;
const errCode = RepStatus.getErrorCode(this.representation);
let errorMessage;
let error;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section changed to allow readable error codes with translated display messages


switch (status) {
case 'error':
switch (RepStatus.getErrorCode(this.representation)) {
switch (errCode) {
case ERROR_PASSWORD_PROTECTED:
errorCode = __('error_password_protected');
errorMessage = __('error_password_protected');
break;
case ERROR_TRY_AGAIN_LATER:
errorCode = __('error_try_again_later');
errorMessage = __('error_try_again_later');
break;
case ERROR_UNSUPPORTED_FORMAT:
errorCode = __('error_bad_file');
errorMessage = __('error_bad_file');
break;
default:
errorCode = __('error_refresh');
errorMessage = __('error_refresh');
break;
}

this.reject(errorCode);
error = new Error(errCode);
error.displayMessage = errorMessage;

this.reject(error);
break;

case STATUS_SUCCESS:
Expand Down
9 changes: 9 additions & 0 deletions src/lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,14 @@ export const VIEWER_EVENT = {
notificationShow: 'notificationshow', // Show notification modal.
notificationHide: 'notificationhide', // Hide notification modal.
mediaEndAutoplay: 'mediaendautoplay', // Media playback has completed, with autoplay enabled.
error: 'error', // When an error occurs
default: 'viewerevent' // The default viewer event
};

export const ERROR_CODE = {
annotationsLoadFail: 'error_annotations_load',
invalidCacheAttempt: 'error_invalid_file_for_cache',
prefetchFileId: 'error_prefetch_file_id',
rateLimit: 'error_rate_limit',
retriesExceeded: 'error_retries_exceeded'
};
11 changes: 11 additions & 0 deletions src/lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,3 +837,14 @@ export function getClosestPageToPinch(x, y, visiblePages) {

return closestPage;
}

/**
* Strip out auth related fields from a string.
*
* @param {string} string - A string containing any auth related fields.
* @return {string} A string with [FILTERED] replacing any auth related fields.
*/
export function stripAuthFromString(string) {
// Strip out "access_token"
return string.replace(/access_token=([^&]*)/, 'access_token=[FILTERED]');
}
16 changes: 8 additions & 8 deletions src/lib/viewers/BaseViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import {
STATUS_VIEWABLE
} from '../constants';
import { getIconFromExtension, getIconFromName } from '../icons/icons';
import { VIEWER_EVENT } from '../events';
import { VIEWER_EVENT, ERROR_CODE } from '../events';

const ANNOTATIONS_JS = 'annotations.js';
const ANNOTATIONS_CSS = 'annotations.css';
Expand Down Expand Up @@ -395,11 +395,7 @@ class BaseViewer extends EventEmitter {
}

if (this.annotationsLoadPromise) {
this.annotationsLoadPromise.then(this.annotationsLoadHandler).catch((err) => {
/* eslint-disable no-console */
console.error('Annotation assets failed to load', err);
/* eslint-enable no-console */
});
this.annotationsLoadPromise.then(this.annotationsLoadHandler);
}
}

Expand Down Expand Up @@ -719,8 +715,12 @@ class BaseViewer extends EventEmitter {
const viewerOptions = {};
viewerOptions[this.options.viewer.NAME] = this.viewerConfig;

/* global BoxAnnotations */
const boxAnnotations = this.options.boxAnnotations || new BoxAnnotations(viewerOptions);
if (!global.BoxAnnotations) {
this.triggerError(ERROR_CODE.annotationsLoadFail);
return;
}

const boxAnnotations = this.options.boxAnnotations || new global.BoxAnnotations(viewerOptions);
this.annotatorConf = boxAnnotations.determineAnnotator(this.options, this.viewerConfig);

if (this.annotatorConf) {
Expand Down
3 changes: 2 additions & 1 deletion src/lib/viewers/error/PreviewErrorViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { PERMISSION_DOWNLOAD } from '../../constants';
import { getIconFromExtension, getIconFromName } from '../../icons/icons';
import './PreviewError.scss';
import { VIEWER_EVENT } from '../../events';
import { stripAuthFromString } from '../../util';

class PreviewErrorViewer extends BaseViewer {
/**
Expand Down Expand Up @@ -102,7 +103,7 @@ class PreviewErrorViewer extends BaseViewer {
const errorMsg = err.message || displayMessage;

// Filter out any access tokens
const filteredMsg = errorMsg.replace(/access_token=([^&]*)/, 'access_token=[FILTERED]');
const filteredMsg = stripAuthFromString(errorMsg);

this.emit(VIEWER_EVENT.load, {
error: filteredMsg
Expand Down