Don't retry flushing CloudWatch metrics (#720)

* 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.

Co-authored-by: Oliver Brook <[email protected]>

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)
 

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)
         })
     })

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 = 3
+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,18 @@ 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)
+            )
         })
     }
 

packages/pwa-kit-runtime/src/utils/ssr-server/utils.js

@@ -14,16 +14,14 @@ 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 getBundleBaseUrl = () => {
-    return `/mobify/bundle/${isRemote() ? BUNDLE_ID : 'development'}/`
+    return `/mobify/bundle/${isRemote() ? process.env.BUNDLE_ID : 'development'}/`
 }
 
 let QUIET = false

packages/pwa-kit-runtime/src/utils/ssr-server/utils.test.js

@@ -0,0 +1,62 @@
+/*
+ * 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) => {
+    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
+        jest.restoreAllMocks()
+    })
+
+    test(`getBundleBaseUrl should return the correct URL`, () => {
+        const expectedId = isRemote ? bundleId : 'development'
+        const expected = `/mobify/bundle/${expectedId}/`
+        expect(utils.getBundleBaseUrl()).toBe(expected)
+    })
+
+    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)
+        })
+    })
+})