Merge pull request #1318 from mozilla/drop-invalid-flow-ids

feat(metrics): Drop invalid flowids so they dont confuse our metrics.

config/index.js

@@ -394,12 +394,14 @@ var conf = convict({
     flow_id_key: {
       default: 'YOU MUST CHANGE ME',
       doc: 'FlowId validation key, as used by content-server',
-      format: String
+      format: String,
+      env: 'FLOW_ID_KEY'
     },
     flow_id_expiry: {
       doc: 'Time after which flowIds are considered stale.',
       format: 'duration',
-      default: '30 minutes'
+      default: '30 minutes',
+      env: 'FLOW_ID_EXPIRY'
     }
   },
   corsOrigin: {

lib/metrics/context.js

@@ -217,6 +217,10 @@ module.exports = function (log, config) {
   }
 
   function logInvalidContext(request, reason) {
+    if (request.payload.metricsContext) {
+      delete request.payload.metricsContext.flowId
+      delete request.payload.metricsContext.flowBeginTime
+    }
     log.warn({
       op: 'metrics.context.validate',
       valid: false,

test/local/metrics_context_tests.js

@@ -654,6 +654,7 @@ test(
     var valid = metricsContext.validate(mockRequest)
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowBeginTime, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -691,6 +692,7 @@ test(
     var valid = metricsContext.validate(mockRequest)
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -729,6 +731,7 @@ test(
     var valid = metricsContext.validate(mockRequest)
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -767,6 +770,7 @@ test(
     var valid = metricsContext.validate(mockRequest)
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -815,6 +819,7 @@ test(
     }
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -863,6 +868,7 @@ test(
     }
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({
@@ -911,6 +917,7 @@ test(
     }
 
     t.notOk(valid, 'the data is treated as invalid')
+    t.notOk(mockRequest.payload.metricsContext.flowId, 'the invalid flow data was removed')
     t.equal(mockLog.info.callCount, 0, 'log.info was not called')
     t.equal(mockLog.warn.callCount, 1, 'log.warn was called once')
     t.ok(mockLog.warn.calledWithExactly({