From 3c0bb2524946e1811516941305bdcced746e1b5a Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 20 Jul 2022 09:32:05 +0100 Subject: [PATCH] Merge pull request #8886 from overleaf/jpa-pdf-caching-alpha [web] prepare alpha release of pdf caching GitOrigin-RevId: 5617dd443da57b7077db793c2bc39be35ec44ef1 --- .../src/Features/Compile/CompileController.js | 137 +++++++++++------- .../src/Features/Project/ProjectController.js | 38 ++--- .../web/app/views/project/editor/meta.pug | 2 - .../js/features/pdf-preview/util/compiler.js | 3 +- .../js/features/pdf-preview/util/metrics.js | 4 +- .../features/pdf-preview/util/output-files.js | 3 +- .../pdf-preview/util/pdf-caching-flags.js | 8 + .../pdf-preview/util/pdf-caching-transport.js | 9 +- .../src/Project/ProjectControllerTests.js | 71 --------- 9 files changed, 118 insertions(+), 157 deletions(-) create mode 100644 services/web/frontend/js/features/pdf-preview/util/pdf-caching-flags.js diff --git a/services/web/app/src/Features/Compile/CompileController.js b/services/web/app/src/Features/Compile/CompileController.js index 43a2950a60..743aa33d98 100644 --- a/services/web/app/src/Features/Compile/CompileController.js +++ b/services/web/app/src/Features/Compile/CompileController.js @@ -1,4 +1,5 @@ let CompileController +const { URL } = require('url') const OError = require('@overleaf/o-error') const Metrics = require('@overleaf/metrics') const ProjectGetter = require('../Project/ProjectGetter') @@ -14,6 +15,7 @@ const ClsiCookieManager = require('./ClsiCookieManager')( ) const Path = require('path') const AnalyticsManager = require('../Analytics/AnalyticsManager') +const SplitTestHandler = require('../SplitTests/SplitTestHandler') const COMPILE_TIMEOUT_MS = 10 * 60 * 1000 @@ -25,18 +27,44 @@ function getImageNameForProject(projectId, callback) { }) } +function getEnablePdfCaching(req, res, cb) { + if (!req.query.enable_pdf_caching) { + // The frontend does not want to do pdf caching. + return cb(null, false) + } + + // Use the query flags from the editor request for overriding the split test. + let query = {} + try { + const u = new URL(req.headers.referer || req.url, Settings.siteUrl) + query = Object.fromEntries(u.searchParams.entries()) + } catch (e) {} + const editorReq = { ...req, query } + + // Double check with the latest split test assignment. + // We may need to turn off the feature on a short notice, without requiring + // all users to reload their editor page to disable the feature. + SplitTestHandler.getAssignment( + editorReq, + res, + 'pdf-caching-mode', + (err, assignment) => { + if (err) return cb(null, false) + cb(null, assignment?.variant === 'enabled') + } + ) +} + module.exports = CompileController = { compile(req, res, next) { res.setTimeout(COMPILE_TIMEOUT_MS) const projectId = req.params.Project_id const isAutoCompile = !!req.query.auto_compile - const enablePdfCaching = !!req.query.enable_pdf_caching const fileLineErrors = !!req.query.file_line_errors const stopOnFirstError = !!req.body.stopOnFirstError const userId = SessionManager.getLoggedInUserId(req.session) const options = { isAutoCompile, - enablePdfCaching, fileLineErrors, stopOnFirstError, } @@ -63,63 +91,68 @@ module.exports = CompileController = { options.incrementalCompilesEnabled = true } - CompileManager.compile( - projectId, - userId, - options, - ( - error, - status, - outputFiles, - clsiServerId, - limits, - validationProblems, - stats, - timings, - outputUrlPrefix - ) => { - if (error) { - Metrics.inc('compile-error') - return next(error) - } - Metrics.inc('compile-status', 1, { status }) - let pdfDownloadDomain = Settings.pdfDownloadDomain - if (pdfDownloadDomain && outputUrlPrefix) { - pdfDownloadDomain += outputUrlPrefix - } + getEnablePdfCaching(req, res, (err, enablePdfCaching) => { + if (err) return next(err) + options.enablePdfCaching = enablePdfCaching - if (limits) { - // For a compile request to be sent to clsi we need limits. - // If we get here without having the limits object populated, it is - // a reasonable assumption to make that nothing was compiled. - // We need to know the limits in order to make use of the events. - AnalyticsManager.recordEventForSession( - req.session, - 'compile-result-backend', - { - projectId, - ownerAnalyticsId: limits.ownerAnalyticsId, - status, - compileTime: timings?.compileE2E, - timeout: limits.timeout === 60 ? 'short' : 'long', - server: clsiServerId?.includes('-c2d-') ? 'faster' : 'normal', - isAutoCompile, - stopOnFirstError, - } - ) - } - res.json({ + CompileManager.compile( + projectId, + userId, + options, + ( + error, status, outputFiles, - compileGroup: limits?.compileGroup, clsiServerId, + limits, validationProblems, stats, timings, - pdfDownloadDomain, - }) - } - ) + outputUrlPrefix + ) => { + if (error) { + Metrics.inc('compile-error') + return next(error) + } + Metrics.inc('compile-status', 1, { status }) + let pdfDownloadDomain = Settings.pdfDownloadDomain + if (pdfDownloadDomain && outputUrlPrefix) { + pdfDownloadDomain += outputUrlPrefix + } + + if (limits) { + // For a compile request to be sent to clsi we need limits. + // If we get here without having the limits object populated, it is + // a reasonable assumption to make that nothing was compiled. + // We need to know the limits in order to make use of the events. + AnalyticsManager.recordEventForSession( + req.session, + 'compile-result-backend', + { + projectId, + ownerAnalyticsId: limits.ownerAnalyticsId, + status, + compileTime: timings?.compileE2E, + timeout: limits.timeout === 60 ? 'short' : 'long', + server: clsiServerId?.includes('-c2d-') ? 'faster' : 'normal', + isAutoCompile, + stopOnFirstError, + } + ) + } + res.json({ + status, + outputFiles, + compileGroup: limits?.compileGroup, + clsiServerId, + validationProblems, + stats, + timings, + pdfDownloadDomain, + }) + } + ) + }) }, stopCompile(req, res, next) { diff --git a/services/web/app/src/Features/Project/ProjectController.js b/services/web/app/src/Features/Project/ProjectController.js index c50c41353c..b34f00b345 100644 --- a/services/web/app/src/Features/Project/ProjectController.js +++ b/services/web/app/src/Features/Project/ProjectController.js @@ -983,6 +983,18 @@ const ProjectController = { } ) }, + trackPdfDownloadAssignment(cb) { + SplitTestHandler.getAssignment(req, res, 'track-pdf-download', () => { + // We'll pick up the assignment from the res.locals assignment. + cb() + }) + }, + pdfCachingModeAssignment(cb) { + SplitTestHandler.getAssignment(req, res, 'pdf-caching-mode', () => { + // We'll pick up the assignment from the res.locals assignment. + cb() + }) + }, }, ( err, @@ -1071,30 +1083,6 @@ const ProjectController = { } } - function getTrackPdfDownload() { - if (!Settings.enablePdfCaching) { - // The feature is disabled globally. - return false - } - // Let the user opt-in only. - // The flag will opt-out of both caching and metrics collection, - // as if this editing session never happened. - return shouldDisplayFeature('track-pdf-download', false) - } - - function getPdfCachingMode() { - if (!Settings.enablePdfCaching) { - // The feature is disabled globally. - return '' - } - // Let the user opt-in only. - const v = req.query['pdf-caching-mode'] - if (['enabled'].includes(v)) { - return v - } - return '' - } - const showPdfDetach = shouldDisplayFeature( 'pdf_detach', pdfDetachAssignment.variant === 'enabled' @@ -1199,8 +1187,6 @@ const ProjectController = { showNewSourceEditorOption, showSymbolPalette, showStopOnFirstError, - trackPdfDownload: getTrackPdfDownload(), - pdfCachingMode: getPdfCachingMode(), detachRole, metadata: { viewport: false }, showHeaderUpgradePrompt, diff --git a/services/web/app/views/project/editor/meta.pug b/services/web/app/views/project/editor/meta.pug index 6199d545bc..ef6b6243e0 100644 --- a/services/web/app/views/project/editor/meta.pug +++ b/services/web/app/views/project/editor/meta.pug @@ -26,8 +26,6 @@ meta(name="ol-showPdfDetach" data-type="boolean" content=showPdfDetach) meta(name="ol-debugPdfDetach" data-type="boolean" content=debugPdfDetach) meta(name="ol-showNewSourceEditorOption" data-type="boolean" content=showNewSourceEditorOption) meta(name="ol-showSymbolPalette" data-type="boolean" content=showSymbolPalette) -meta(name="ol-pdfCachingMode" content=pdfCachingMode) -meta(name="ol-trackPdfDownload" data-type="boolean" content=trackPdfDownload) meta(name="ol-detachRole" data-type="string" content=detachRole) meta(name="ol-showHeaderUpgradePrompt" data-type="boolean" content=showHeaderUpgradePrompt) meta(name="ol-showStopOnFirstError" data-type="boolean" content=showStopOnFirstError) diff --git a/services/web/frontend/js/features/pdf-preview/util/compiler.js b/services/web/frontend/js/features/pdf-preview/util/compiler.js index f0736ce723..78aa38e32c 100644 --- a/services/web/frontend/js/features/pdf-preview/util/compiler.js +++ b/services/web/frontend/js/features/pdf-preview/util/compiler.js @@ -3,6 +3,7 @@ import getMeta from '../../../utils/meta' import { deleteJSON, postJSON } from '../../../infrastructure/fetch-json' import { debounce } from 'lodash' import { trackPdfDownload } from './metrics' +import { enablePdfCaching } from './pdf-caching-flags' const AUTO_COMPILE_MAX_WAIT = 5000 // We add a 2 second debounce to sending user changes to server if they aren't @@ -168,7 +169,7 @@ export default class DocumentCompiler { } // use the feature flag to enable PDF caching - if (getMeta('ol-pdfCachingMode') === 'enabled') { + if (enablePdfCaching) { params.set('enable_pdf_caching', 'true') } diff --git a/services/web/frontend/js/features/pdf-preview/util/metrics.js b/services/web/frontend/js/features/pdf-preview/util/metrics.js index 962b6ea01b..444c604efc 100644 --- a/services/web/frontend/js/features/pdf-preview/util/metrics.js +++ b/services/web/frontend/js/features/pdf-preview/util/metrics.js @@ -1,6 +1,6 @@ import { v4 as uuid } from 'uuid' import { sendMB } from '../../../infrastructure/event-tracking' -import getMeta from '../../../utils/meta' +import { trackPdfDownloadEnabled } from './pdf-caching-flags' // VERSION should get incremented when making changes to caching behavior or // adjusting metrics collection. @@ -39,7 +39,7 @@ export function trackPdfDownload(response, compileTimeClientE2E, t0) { if (latencyRender) { deliveryLatencies.latencyRender = latencyRender } - if (getMeta('ol-trackPdfDownload')) { + if (trackPdfDownloadEnabled) { // Submit latency along with compile context. submitCompileMetrics({ totalDeliveryTime, diff --git a/services/web/frontend/js/features/pdf-preview/util/output-files.js b/services/web/frontend/js/features/pdf-preview/util/output-files.js index a1d64cb543..e1d04eff19 100644 --- a/services/web/frontend/js/features/pdf-preview/util/output-files.js +++ b/services/web/frontend/js/features/pdf-preview/util/output-files.js @@ -2,6 +2,7 @@ import getMeta from '../../../utils/meta' import HumanReadableLogs from '../../../ide/human-readable-logs/HumanReadableLogs' import BibLogParser from '../../../ide/log-parser/bib-log-parser' import { v4 as uuid } from 'uuid' +import { enablePdfCaching } from './pdf-caching-flags' // Warnings that may disappear after a second LaTeX pass const TRANSIENT_WARNING_REGEX = /^(Reference|Citation).+undefined on input line/ @@ -19,7 +20,7 @@ export function handleOutputFiles(outputFiles, projectId, data) { params.set('clsiserverid', data.clsiServerId) } - if (getMeta('ol-pdfCachingMode') === 'enabled') { + if (enablePdfCaching) { // Tag traffic that uses the pdf caching logic. params.set('enable_pdf_caching', 'true') } diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-caching-flags.js b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-flags.js new file mode 100644 index 0000000000..484b7e29b1 --- /dev/null +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-flags.js @@ -0,0 +1,8 @@ +import getMeta from '../../../utils/meta' + +function isFlagEnabled(flag) { + return getMeta('ol-splitTestVariants')?.[flag] === 'enabled' +} + +export const enablePdfCaching = isFlagEnabled('pdf-caching-mode') +export const trackPdfDownloadEnabled = isFlagEnabled('track-pdf-download') diff --git a/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js index b5989c6e87..f3148cd660 100644 --- a/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js +++ b/services/web/frontend/js/features/pdf-preview/util/pdf-caching-transport.js @@ -1,10 +1,11 @@ import { fallbackRequest, fetchRange } from './pdf-caching' -import getMeta from '../../../utils/meta' import { captureException } from '../../../infrastructure/error-reporter' import { getPdfCachingMetrics } from './metrics' +import { enablePdfCaching, trackPdfDownloadEnabled } from './pdf-caching-flags' export function generatePdfCachingTransportFactory(PDFJS) { - if (getMeta('ol-pdfCachingMode') !== 'enabled') { + // NOTE: The custom transport can be used for tracking download volume. + if (!enablePdfCaching && !trackPdfDownloadEnabled) { return () => null } let failedOnce = false @@ -19,6 +20,7 @@ export function generatePdfCachingTransportFactory(PDFJS) { fetchedBytes: 0, requestedCount: 0, requestedBytes: 0, + enablePdfCaching, }) const verifyChunks = new URLSearchParams(window.location.search).get('verify_chunks') === 'true' @@ -51,6 +53,9 @@ export function generatePdfCachingTransportFactory(PDFJS) { .catch(err => { metrics.failedCount++ failedOnce = true + if (!enablePdfCaching) { + throw err // This was a fallback request already. Do not retry. + } console.error('optimized pdf download error', err) captureException(err) return fallbackRequest({ url: this.url, start, end, abortSignal }) diff --git a/services/web/test/unit/src/Project/ProjectControllerTests.js b/services/web/test/unit/src/Project/ProjectControllerTests.js index 9046f0b1af..d2c4b972a4 100644 --- a/services/web/test/unit/src/Project/ProjectControllerTests.js +++ b/services/web/test/unit/src/Project/ProjectControllerTests.js @@ -1126,77 +1126,6 @@ describe('ProjectController', function () { this.ProjectController.loadEditor(this.req, this.res) }) - describe('pdf caching feature flags', function () { - function expectBandwidthTrackingEnabled() { - it('should track pdf bandwidth', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.trackPdfDownload).to.equal(true) - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - } - function expectPDFCachingEnabled(mode) { - it('should enable pdf caching', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.pdfCachingMode).to.equal(mode) - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - } - function expectBandwidthTrackingDisabled() { - it('should not track pdf bandwidth', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.trackPdfDownload).to.equal(false) - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - } - function expectPDFCachingDisabled() { - it('should disable pdf caching', function (done) { - this.res.render = (pageName, opts) => { - expect(opts.pdfCachingMode).to.equal('') - done() - } - this.ProjectController.loadEditor(this.req, this.res) - }) - } - - beforeEach(function () { - this.settings.enablePdfCaching = true - }) - - describe('during opt-in only', function () { - describe('with no query', function () { - expectBandwidthTrackingDisabled() - expectPDFCachingDisabled() - }) - - describe('with track-pdf-download=false', function () { - beforeEach(function () { - this.req.query['track-pdf-download'] = 'false' - }) - expectBandwidthTrackingDisabled() - }) - - describe('with track-pdf-download=true', function () { - beforeEach(function () { - this.req.query['track-pdf-download'] = 'true' - }) - expectBandwidthTrackingEnabled() - }) - - describe('with pdf-caching-mode=enabled', function () { - beforeEach(function () { - this.req.query['pdf-caching-mode'] = 'enabled' - }) - expectPDFCachingEnabled('enabled') - }) - }) - }) - describe('wsUrl', function () { function checkLoadEditorWsMetric(metric) { it(`should inc metric ${metric}`, function (done) {