diff --git a/src/controllers/brands.js b/src/controllers/brands.js index cb7a5b6ab6..f3f1821fb1 100644 --- a/src/controllers/brands.js +++ b/src/controllers/brands.js @@ -153,7 +153,7 @@ function BrandsController(ctx, log, env) { Product: context?.pathInfo?.headers?.['x-product'], }, }, - { environment: resolveEnvironment(env), namespace: BRAND_METRICS_NAMESPACE }, + { environment: resolveEnvironment(env, { log }), namespace: BRAND_METRICS_NAMESPACE }, ); log.warn(`BrandDemotionBlocked: ${operation} attempted an active->pending demotion ` + `(org=${context?.params?.spaceCatId}, brand=${context?.params?.brandId}, ` diff --git a/src/controllers/redirects.js b/src/controllers/redirects.js index 5793eafcfc..3a5a8c8e49 100644 --- a/src/controllers/redirects.js +++ b/src/controllers/redirects.js @@ -141,7 +141,7 @@ function RedirectsController(ctx) { // Capture start time up front so every terminal branch reports duration. const startedAt = Date.now(); const emitOpts = { - environment: resolveEnvironment(env), + environment: resolveEnvironment(env, { log }), namespace: ASO_OVERLAY_NAMESPACE, }; diff --git a/src/controllers/webhooks.js b/src/controllers/webhooks.js index 85469670eb..cedf1c696a 100644 --- a/src/controllers/webhooks.js +++ b/src/controllers/webhooks.js @@ -83,7 +83,7 @@ function WebhooksController(context) { return await fn(ctx); } catch (e) { log.error('GitHub webhook handler error', e); - emitMetric({ name: 'WebhookHandlerError', dimensions: { Reason: 'uncaught_exception' } }, { environment: resolveEnvironment(env) }); + emitMetric({ name: 'WebhookHandlerError', dimensions: { Reason: 'uncaught_exception' } }, { environment: resolveEnvironment(env, { log }) }); return internalServerError('Internal error'); } }; @@ -98,7 +98,7 @@ function WebhooksController(context) { const deliveryId = ctx.pathInfo?.headers?.['x-github-delivery']; const { data } = ctx; - const environment = resolveEnvironment(env); + const environment = resolveEnvironment(env, { log }); const startedAt = Date.now(); let outcome = 'unknown'; diff --git a/src/support/aso-overlay-key-handler.js b/src/support/aso-overlay-key-handler.js index b8b5f4b9a8..6678e9f87f 100644 --- a/src/support/aso-overlay-key-handler.js +++ b/src/support/aso-overlay-key-handler.js @@ -75,7 +75,7 @@ class AsoOverlayKeyHandler extends AbstractHandler { // From here on, we're on the overlay route — every outcome is worth a metric. const emitOpts = { - environment: resolveEnvironment(context.env), + environment: resolveEnvironment(context.env, { log: context.log }), namespace: ASO_OVERLAY_NAMESPACE, }; diff --git a/src/support/github-webhook-hmac-handler.js b/src/support/github-webhook-hmac-handler.js index e5f7cc8566..d4bdfce6f7 100644 --- a/src/support/github-webhook-hmac-handler.js +++ b/src/support/github-webhook-hmac-handler.js @@ -80,7 +80,7 @@ class GitHubWebhookHmacHandler extends AbstractHandler { // Best-effort metric helpers — defined here so context.env is in scope. // Both swallow errors (emitMetric is itself best-effort, and these are // called on auth-rejection paths where we must never throw). - const environment = resolveEnvironment(context.env); + const environment = resolveEnvironment(context.env, { log: context.log }); const rejected = (reason) => emitMetric( { name: 'WebhookRejected', dimensions: { Reason: reason } }, { environment }, diff --git a/src/support/metrics-emf.js b/src/support/metrics-emf.js index 7a6b69f15f..5421935ae1 100644 --- a/src/support/metrics-emf.js +++ b/src/support/metrics-emf.js @@ -24,15 +24,59 @@ const NAMESPACE = 'Mysticat/GitHubService'; +// Module-scoped flag so we warn at most once per Lambda instance when the +// environment falls back to the default. Emitting per-request would flood the +// log for every invocation on a mis-configured deploy; once-per-instance is +// enough for ops to notice on the first cold start and still avoid confusing +// on-call with a warn spike that dwarfs the actual outcome events. +let envFallbackWarned = false; + /** * Resolves the deployment environment name from process/Lambda env vars. * Precedence: AWS_ENV > ENV > 'dev'. * + * If neither variable is set the caller falls back to 'dev'. In prod that is a + * silent mis-labeling — every metric routes to the dev dashboard while prod + * dashboards read as zero traffic. Pass `log` to have this warned once per + * Lambda instance (via `log.warn`) so a broken deploy manifest surfaces on the + * very first invocation rather than staying invisible until someone spot-checks + * the wrong-tier dashboard. + * * @param {object} env - The env object (context.env or process.env) + * @param {object} [opts] - Options + * @param {object} [opts.log] - Optional logger for the default-fallback warning * @returns {string} Environment name */ -export function resolveEnvironment(env = {}) { - return env.AWS_ENV || env.ENV || 'dev'; +export function resolveEnvironment(env = {}, { log } = {}) { + if (env.AWS_ENV) { + return env.AWS_ENV; + } + if (env.ENV) { + return env.ENV; + } + // Defensive: some callers pass a partial logger (info/error only) — helix + // and Lambda contexts historically only guarantee `log.info`, so `log.warn` + // may be absent. Skip silently rather than crash the request path. + if (log && typeof log.warn === 'function' && !envFallbackWarned) { + envFallbackWarned = true; + log.warn( + '[metrics-emf] resolveEnvironment: neither AWS_ENV nor ENV is set — ' + + "defaulting to 'dev'. In a production deploy this mis-labels metrics; " + + 'check the Lambda env-var manifest.', + ); + } + return 'dev'; +} + +/** + * Test-only: reset the module-scoped warn-once latch so unit tests can verify + * the first-call semantics without cross-test bleed. Exported (not prefixed + * with an underscore, which lint forbids) but named to make its scope obvious + * so no production caller reaches for it by mistake. + */ +// eslint-disable-next-line camelcase +export function resetEnvFallbackWarnedForTest() { + envFallbackWarned = false; } /** diff --git a/test/support/metrics-emf.test.js b/test/support/metrics-emf.test.js index 48228d046b..2644e034dd 100644 --- a/test/support/metrics-emf.test.js +++ b/test/support/metrics-emf.test.js @@ -11,7 +11,12 @@ */ import { expect } from 'chai'; -import { emitMetric, resolveEnvironment } from '../../src/support/metrics-emf.js'; +import sinon from 'sinon'; +import { + emitMetric, + resolveEnvironment, + resetEnvFallbackWarnedForTest, +} from '../../src/support/metrics-emf.js'; describe('metrics-emf', () => { it('resolveEnvironment prefers AWS_ENV, then ENV, then dev', () => { @@ -20,6 +25,41 @@ describe('metrics-emf', () => { expect(resolveEnvironment({})).to.equal('dev'); }); + describe('resolveEnvironment: default-fallback warn-once', () => { + beforeEach(() => resetEnvFallbackWarnedForTest()); + afterEach(() => resetEnvFallbackWarnedForTest()); + + it('does NOT warn when AWS_ENV is set', () => { + const log = { warn: sinon.spy() }; + expect(resolveEnvironment({ AWS_ENV: 'prod' }, { log })).to.equal('prod'); + expect(log.warn.called).to.be.false; + }); + + it('does NOT warn when ENV is set (AWS_ENV missing)', () => { + const log = { warn: sinon.spy() }; + expect(resolveEnvironment({ ENV: 'stage' }, { log })).to.equal('stage'); + expect(log.warn.called).to.be.false; + }); + + it('warns exactly once per Lambda instance when defaulting to dev', () => { + const log = { warn: sinon.spy() }; + // First call: log.warn fires. + expect(resolveEnvironment({}, { log })).to.equal('dev'); + expect(log.warn.callCount).to.equal(1); + expect(log.warn.firstCall.args[0]).to.include('AWS_ENV nor ENV'); + // Second call same instance: still 'dev', but no additional warn — the + // guard prevents flooding on every request when the manifest is broken. + expect(resolveEnvironment({}, { log })).to.equal('dev'); + expect(log.warn.callCount).to.equal(1); + }); + + it('is a no-op when no log is passed (backwards compat with old callers)', () => { + // Legacy callers pass no options; the function must still return 'dev' + // without throwing, and must not require the log param. + expect(resolveEnvironment({})).to.equal('dev'); + }); + }); + it('emits a well-formed EMF envelope to the injected sink', () => { const lines = []; emitMetric(