From dee0af8536a841ec41249409192b150a794f2298 Mon Sep 17 00:00:00 2001 From: Jenn Mills Date: Thu, 15 Sep 2022 12:50:13 -0700 Subject: [PATCH 1/6] Don't retry flushing CloudWatch metrics The api for flushing metrics is rate limited, in high load environments this may cause performance issues as we wait for this call to finish so if it fails we will not retry. --- packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js index e711f44977..68a525a557 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js @@ -6,7 +6,7 @@ */ import {isRemote, localDevLog} from './utils' -const METRIC_RETRIES = 3 +const METRIC_RETRIES = 0 /** * A class that will handle asynchronous sending of CloudWatch From 4c658fa39f61962964e3f72a766c4cc2fd63c57b Mon Sep 17 00:00:00 2001 From: Oliver Brook Date: Tue, 4 Oct 2022 20:38:18 -0700 Subject: [PATCH 2/6] Remove retry logic for CloudWatch and boost branch coverage --- .../src/utils/ssr-server.test.js | 9 +-- .../src/utils/ssr-server/cached-response.js | 1 + .../src/utils/ssr-server/metrics-sender.js | 66 +++---------------- .../src/utils/ssr-server/utils.js | 7 +- .../src/utils/ssr-server/utils.test.js | 29 ++++++++ 5 files changed, 41 insertions(+), 71 deletions(-) create mode 100644 packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server.test.js b/packages/pwa-kit-runtime/src/utils/ssr-server.test.js index f01b54e55f..887430f795 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server.test.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server.test.js @@ -669,15 +669,8 @@ describe('MetricsSender', () => { sender.send(metrics1) expect(sender.queueLength).toEqual(metrics1.length) return sender.flush().then(() => { - const expectedAttempts = sender._retries - // Expect that we retried the correct number of times - expect(sender._CW.putMetricData.callCount).toBe(expectedAttempts) - - // Expect that there was a console warning for each retry - expect(consoleWarn.callCount).toBe(expectedAttempts) - - // Expect that the MetricsSender is now empty + expect(sender._CW.putMetricData.callCount).toBe(1) expect(sender.queueLength).toBe(0) }) }) diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js b/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js index a4a365b12e..43aa8896dc 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js @@ -1,3 +1,4 @@ + /* * Copyright (c) 2022, Salesforce, Inc. * All rights reserved. diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js index 68a525a557..523fe40cb1 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js @@ -4,9 +4,7 @@ * SPDX-License-Identifier: BSD-3-Clause * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause */ -import {isRemote, localDevLog} from './utils' - -const METRIC_RETRIES = 0 +import {isRemote} from './utils' /** * A class that will handle asynchronous sending of CloudWatch @@ -27,9 +25,6 @@ export class MetricsSender { // name/value metric, and they accumulate on this queue // until batched up into a putMetricData call. this._queue = [] - - // The default retry count - this._retries = METRIC_RETRIES } /** @@ -50,9 +45,6 @@ export class MetricsSender { _setup() { /* istanbul ignore next */ if (!this._CW && (isRemote() || MetricsSender._override)) { - // Load the AWS sdk. Although we do this on-demand, it's - // likely that a webpacked project will include the whole - // required module, so we require just the client that we need. const Cloudwatch = require('aws-sdk/clients/cloudwatch') this._CW = new Cloudwatch({ apiVersion: '2010-08-01', @@ -80,59 +72,17 @@ export class MetricsSender { } return new Promise((resolve) => { - // The parameters for putMetricData - const params = { - MetricData: metrics, - Namespace: 'ssr' - } - - // Initialize the retry count - let retries = this._retries - - // Initialize the retry delay - // This delay is long enough to reduce the sending - // rate, but not so much that it delays completion - // of a response too much. - let retryDelay = 50 // mS - - // This callback is called when the putMetricData operation - // is complete, or if an error occurs. - const callback = (err) => { - let retry = false - - if (err) { - if (err.code === 'Throttling') { - // We exceeded the CloudWatch metric sending - // rate. We may need to retry. - retry = --retries > 0 - const msg = retry ? `retrying after ${retryDelay} mS` : 'not retrying' - console.warn( - `Metrics: ${err} when sending metric data (length ${metrics.length}): ${msg}` - ) - } else { - // Some other error + cw.putMetricData( + { + MetricData: metrics, + Namespace: 'ssr' + }, (err) => { + if (err) { console.warn(`Metrics: error sending data: ${err}`) } - } - - if (retry) { - // We need to delay before we actually retry - setTimeout(() => cw.putMetricData(params, callback), retryDelay) - // Increase the delay for any future retry - retryDelay += 25 // mS - } else { - // Sending is done. - localDevLog( - `Metrics: ${err ? 'discarded' : 'completed'} sending ${ - metrics.length - } metric(s)` - ) resolve() } - } - - // Send the metrics - cw.putMetricData(params, callback) + ) }) } diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js index ef60cc6917..55d7dcf6c4 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js @@ -14,16 +14,13 @@ import crypto from 'crypto' import {PROXY_PATH_PREFIX} from '../../ssr/server/constants' import {proxyConfigs} from '../ssr-shared' -const BUNDLE_ID = process.env.BUNDLE_ID - // TODO: Clean this up or provide a way to toggle export const verboseProxyLogging = false -export const isRemote = () => - Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME') +export const isRemote = () => Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME') export const getBundleBaseUrl = () => { - return `/mobify/bundle/${isRemote() ? BUNDLE_ID : 'development'}/` + return `/mobify/bundle/${isRemote() ? process.env.BUNDLE_ID : 'development'}/` } let QUIET = false diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js new file mode 100644 index 0000000000..133a969304 --- /dev/null +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js @@ -0,0 +1,29 @@ +import * as utils from './utils' + +describe.each([[true], [false]])('Utils (isRemote: %p)', (isRemote) => { + let originalEnv + const bundleId = 'test-bundle-id-12345' + + beforeEach(() => { + originalEnv = process.env + process.env = Object.assign({}, process.env) + process.env.BUNDLE_ID = bundleId + if (isRemote) { + process.env.AWS_LAMBDA_FUNCTION_NAME = 'remote-test-name' + } + }) + + afterEach(() => { + process.env = originalEnv + }) + + test(`getBundleBaseUrl should return the correct URL`, () => { + const expectedId = isRemote ? bundleId : 'development' + const expected = `/mobify/bundle/${expectedId}/` + expect(utils.getBundleBaseUrl()).toBe(expected) + }) + + test(`localDevLog should log conditionally`, () => { + utils.localDevLog('foo') + }) + }) \ No newline at end of file From cb2b08dc009cf6d9ca450db713d43573cced6e02 Mon Sep 17 00:00:00 2001 From: Oliver Brook Date: Wed, 5 Oct 2022 09:26:02 -0700 Subject: [PATCH 3/6] Format --- .../src/utils/ssr-server/cached-response.js | 1 - .../src/utils/ssr-server/metrics-sender.js | 5 ++- .../src/utils/ssr-server/utils.js | 3 +- .../src/utils/ssr-server/utils.test.js | 44 +++++++++---------- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js b/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js index 43aa8896dc..a4a365b12e 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/cached-response.js @@ -1,4 +1,3 @@ - /* * Copyright (c) 2022, Salesforce, Inc. * All rights reserved. diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js index 523fe40cb1..a9c24605b8 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/metrics-sender.js @@ -76,13 +76,14 @@ export class MetricsSender { { MetricData: metrics, Namespace: 'ssr' - }, (err) => { + }, + (err) => { if (err) { console.warn(`Metrics: error sending data: ${err}`) } resolve() } - ) + ) }) } diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js index 55d7dcf6c4..449649d8e6 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.js @@ -17,7 +17,8 @@ import {proxyConfigs} from '../ssr-shared' // TODO: Clean this up or provide a way to toggle export const verboseProxyLogging = false -export const isRemote = () => Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME') +export const isRemote = () => + Object.prototype.hasOwnProperty.call(process.env, 'AWS_LAMBDA_FUNCTION_NAME') export const getBundleBaseUrl = () => { return `/mobify/bundle/${isRemote() ? process.env.BUNDLE_ID : 'development'}/` diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js index 133a969304..8b8a9a9336 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js @@ -1,29 +1,29 @@ import * as utils from './utils' describe.each([[true], [false]])('Utils (isRemote: %p)', (isRemote) => { - let originalEnv - const bundleId = 'test-bundle-id-12345' + let originalEnv + const bundleId = 'test-bundle-id-12345' - beforeEach(() => { - originalEnv = process.env - process.env = Object.assign({}, process.env) - process.env.BUNDLE_ID = bundleId - if (isRemote) { - process.env.AWS_LAMBDA_FUNCTION_NAME = 'remote-test-name' - } - }) + beforeEach(() => { + originalEnv = process.env + process.env = Object.assign({}, process.env) + process.env.BUNDLE_ID = bundleId + if (isRemote) { + process.env.AWS_LAMBDA_FUNCTION_NAME = 'remote-test-name' + } + }) - afterEach(() => { - process.env = originalEnv - }) + afterEach(() => { + process.env = originalEnv + }) - test(`getBundleBaseUrl should return the correct URL`, () => { - const expectedId = isRemote ? bundleId : 'development' - const expected = `/mobify/bundle/${expectedId}/` - expect(utils.getBundleBaseUrl()).toBe(expected) - }) + test(`getBundleBaseUrl should return the correct URL`, () => { + const expectedId = isRemote ? bundleId : 'development' + const expected = `/mobify/bundle/${expectedId}/` + expect(utils.getBundleBaseUrl()).toBe(expected) + }) - test(`localDevLog should log conditionally`, () => { - utils.localDevLog('foo') - }) - }) \ No newline at end of file + test(`localDevLog should log conditionally`, () => { + utils.localDevLog('foo') + }) +}) From 999a93b65b9df02badd4c72ee305ca72151d1b9a Mon Sep 17 00:00:00 2001 From: Oliver Brook Date: Wed, 5 Oct 2022 10:01:04 -0700 Subject: [PATCH 4/6] Bump coverage --- .../src/utils/ssr-server/utils.test.js | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js index 8b8a9a9336..dca1e2667a 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js @@ -1,6 +1,6 @@ import * as utils from './utils' -describe.each([[true], [false]])('Utils (isRemote: %p)', (isRemote) => { +describe.each([[true], [false]])('Utils remote/local tests (isRemote: %p)', (isRemote) => { let originalEnv const bundleId = 'test-bundle-id-12345' @@ -15,6 +15,7 @@ describe.each([[true], [false]])('Utils (isRemote: %p)', (isRemote) => { afterEach(() => { process.env = originalEnv + jest.restoreAllMocks() }) test(`getBundleBaseUrl should return the correct URL`, () => { @@ -23,7 +24,33 @@ describe.each([[true], [false]])('Utils (isRemote: %p)', (isRemote) => { expect(utils.getBundleBaseUrl()).toBe(expected) }) - test(`localDevLog should log conditionally`, () => { - utils.localDevLog('foo') + describe.each([[true], [false]])('Quiet/loud tests', (quiet) => { + let originalQuiet + + beforeEach(() => { + originalQuiet = utils.isQuiet() + utils.setQuiet(quiet) + }) + + afterEach(() => { + utils.setQuiet(originalQuiet) + jest.restoreAllMocks() + }) + + test(`localDevLog should log conditionally (quiet: ${quiet})`, () => { + const log = jest.spyOn(console, 'log').mockImplementation(() => {}) + const msg = 'message' + utils.localDevLog(msg) + const expected = !isRemote && !quiet ? [[msg]] : [] + expect(log.mock.calls).toEqual(expected) + }) + + test(`infoLog should log conditionally (quiet: ${quiet})`, () => { + const log = jest.spyOn(console, 'log').mockImplementation(() => {}) + const msg = 'message' + utils.infoLog(msg) + const expected = !quiet ? [[msg]] : [] + expect(log.mock.calls).toEqual(expected) + }) }) }) From f3aa9b16938638fd719ea36d341658b754d8ed65 Mon Sep 17 00:00:00 2001 From: Oliver Brook Date: Wed, 5 Oct 2022 10:10:56 -0700 Subject: [PATCH 5/6] Lint --- packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js index dca1e2667a..c7a1db1ab9 100644 --- a/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js +++ b/packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js @@ -1,3 +1,9 @@ +/* + * Copyright (c) 2022, Salesforce, Inc. + * All rights reserved. + * SPDX-License-Identifier: BSD-3-Clause + * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause + */ import * as utils from './utils' describe.each([[true], [false]])('Utils remote/local tests (isRemote: %p)', (isRemote) => { From a579e28fc975a4f7b80644f1dc5a80fba12398c9 Mon Sep 17 00:00:00 2001 From: Oliver Brook Date: Wed, 5 Oct 2022 12:13:55 -0700 Subject: [PATCH 6/6] Changelog --- packages/pwa-kit-runtime/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/pwa-kit-runtime/CHANGELOG.md b/packages/pwa-kit-runtime/CHANGELOG.md index 37c679ccdf..7bbaa8019f 100644 --- a/packages/pwa-kit-runtime/CHANGELOG.md +++ b/packages/pwa-kit-runtime/CHANGELOG.md @@ -1,4 +1,5 @@ ## v2.3.0-dev (Aug 25, 2022) +- Performance: Skip retries when flushing CloudWatch metrics, prioritize returning a response instead. [720](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/720) ## v2.2.0 (Aug 25, 2022) ## v2.1.0 (Jul 05, 2022)