FIX: Prevent errors from uppy performance logging (#20910)

Back in d0e1c222f7 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.
This commit is contained in:
Martin Brennan 2023-03-31 14:29:07 +10:00 committed by GitHub
parent f7997ae882
commit ef1b781ced
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 46 additions and 15 deletions

View File

@ -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`

View File

@ -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