From ef1b781ced2fb9feb29b6fbe5725e3defb6ea064 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 31 Mar 2023 14:29:07 +1000 Subject: [PATCH] FIX: Prevent errors from uppy performance logging (#20910) Back in d0e1c222f75b120ddc763db5e2443bcbfa0ded70 we added performance measuring for uppy uploads using the Performance API in the browser. However we recently discovered that sometimes performance.measure can fail if for whatever reason one of the marks passed to it does not exist: > Failed to upload ... Performance.measure: Given mark name, upload-uppy-....-create-multipart-success, is unknown This would cause the entire upload to fail, which is unnecessary for a debugger. Improve the situation so if this happens again the error does not stop the upload. --- .../discourse/app/mixins/upload-debugging.js | 57 ++++++++++++++----- .../discourse/app/mixins/uppy-upload.js | 4 ++ 2 files changed, 46 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/discourse/app/mixins/upload-debugging.js b/app/assets/javascripts/discourse/app/mixins/upload-debugging.js index 07c6d598547..f1c0cc6f7cf 100644 --- a/app/assets/javascripts/discourse/app/mixins/upload-debugging.js +++ b/app/assets/javascripts/discourse/app/mixins/upload-debugging.js @@ -10,6 +10,12 @@ export default Mixin.create({ }, _consolePerformanceTiming(timing) { + // Sometimes performance.measure can fail to return a PerformanceMeasure + // object, in this case we can't log anything so return to prevent errors. + if (!timing) { + return; + } + const minutes = Math.floor(timing.duration / 60000); const seconds = ((timing.duration % 60000) / 1000).toFixed(0); const duration = minutes + ":" + (seconds < 10 ? "0" : "") + seconds; @@ -19,9 +25,9 @@ export default Mixin.create({ }, _performanceApiSupport() { - performance.mark("testing support 1"); - performance.mark("testing support 2"); - const perfMeasure = performance.measure( + this._performanceMark("testing support 1"); + this._performanceMark("testing support 2"); + const perfMeasure = this._performanceMeasure( "performance api support", "testing support 1", "testing support 2" @@ -29,10 +35,31 @@ export default Mixin.create({ return perfMeasure; }, + _performanceMark(markName) { + return performance.mark(markName); + }, + + _performanceMeasure(measureName, startMark, endMark) { + let measureResult; + try { + measureResult = performance.measure(measureName, startMark, endMark); + } catch (error) { + if ( + error.message.includes("Failed to execute 'measure' on 'Performance'") + ) { + // eslint-disable-next-line no-console + console.warn( + `Uppy performance measure failed: ${measureName}, ${startMark}, ${endMark}` + ); + } + } + return measureResult; + }, + _instrumentUploadTimings() { if (!this._performanceApiSupport()) { warn( - "Some browsers do not return a PerformanceMeasure when calling performance.mark, disabling instrumentation. See https://developer.mozilla.org/en-US/docs/Web/API/Performance/measure#return_value and https://bugzilla.mozilla.org/show_bug.cgi?id=1724645", + "Some browsers do not return a PerformanceMeasure when calling this._performanceMark, disabling instrumentation. See https://developer.mozilla.org/en-US/docs/Web/API/this._performanceMeasure#return_value and https://bugzilla.mozilla.org/show_bug.cgi?id=1724645", { id: "discourse.upload-debugging" } ); return; @@ -40,23 +67,23 @@ export default Mixin.create({ this._uppyInstance.on("upload", (data) => { data.fileIDs.forEach((fileId) => - performance.mark(`upload-${fileId}-start`) + this._performanceMark(`upload-${fileId}-start`) ); }); this._uppyInstance.on("create-multipart", (fileId) => { - performance.mark(`upload-${fileId}-create-multipart`); + this._performanceMark(`upload-${fileId}-create-multipart`); }); this._uppyInstance.on("create-multipart-success", (fileId) => { - performance.mark(`upload-${fileId}-create-multipart-success`); + this._performanceMark(`upload-${fileId}-create-multipart-success`); }); this._uppyInstance.on("complete-multipart", (fileId) => { - performance.mark(`upload-${fileId}-complete-multipart`); + this._performanceMark(`upload-${fileId}-complete-multipart`); this._consolePerformanceTiming( - performance.measure( + this._performanceMeasure( `upload-${fileId}-multipart-all-parts-complete`, `upload-${fileId}-create-multipart-success`, `upload-${fileId}-complete-multipart` @@ -65,10 +92,10 @@ export default Mixin.create({ }); this._uppyInstance.on("complete-multipart-success", (fileId) => { - performance.mark(`upload-${fileId}-complete-multipart-success`); + this._performanceMark(`upload-${fileId}-complete-multipart-success`); this._consolePerformanceTiming( - performance.measure( + this._performanceMeasure( `upload-${fileId}-multipart-total-network-exclusive-complete-multipart`, `upload-${fileId}-create-multipart`, `upload-${fileId}-complete-multipart` @@ -76,7 +103,7 @@ export default Mixin.create({ ); this._consolePerformanceTiming( - performance.measure( + this._performanceMeasure( `upload-${fileId}-multipart-total-network-inclusive-complete-multipart`, `upload-${fileId}-create-multipart`, `upload-${fileId}-complete-multipart-success` @@ -84,7 +111,7 @@ export default Mixin.create({ ); this._consolePerformanceTiming( - performance.measure( + this._performanceMeasure( `upload-${fileId}-multipart-complete-convert-to-upload`, `upload-${fileId}-complete-multipart`, `upload-${fileId}-complete-multipart-success` @@ -93,9 +120,9 @@ export default Mixin.create({ }); this._uppyInstance.on("upload-success", (file) => { - performance.mark(`upload-${file.id}-end`); + this._performanceMark(`upload-${file.id}-end`); this._consolePerformanceTiming( - performance.measure( + this._performanceMeasure( `upload-${file.id}-multipart-total-inclusive-preprocessing`, `upload-${file.id}-start`, `upload-${file.id}-end` diff --git a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js index 6b8b42aac60..64dfb8cb0ce 100644 --- a/app/assets/javascripts/discourse/app/mixins/uppy-upload.js +++ b/app/assets/javascripts/discourse/app/mixins/uppy-upload.js @@ -267,6 +267,10 @@ export default Mixin.create(UppyS3Multipart, ExtendableUploader, { }); }); + if (this.siteSettings.enable_upload_debug_mode) { + this._instrumentUploadTimings(); + } + // TODO (martin) preventDirectS3Uploads is necessary because some of // the current upload mixin components, for example the emoji uploader, // send the upload to custom endpoints that do fancy things in the rails