fix: gateway histogram response time only updated on 200 (#80)

Co-authored-by: Alan Shaw <alan.shaw@protocol.ai>

packages/edge-gateway/src/durable-objects/gateway-metrics.js

@@ -2,14 +2,15 @@ import {
   responseTimeHistogram,
   createResponseTimeHistogramObject,
 } from '../utils/histogram.js'
+import { HTTP_STATUS_SUCCESS } from '../constants.js'
 
 /**
  * @typedef {Object} GatewayMetrics
  * @property {number} totalResponseTime total response time of the requests
  * @property {number} totalWinnerRequests number of performed requests where winner
  * @property {Record<string, number>} totalResponsesByStatus total responses received indexed by status code
  * @property {Record<string, number>} totalRequestsPreventedByReason total requests not sent to upstream gateway indexed by reason code
- * @property {Record<string, number>} responseTimeHistogram
+ * @property {Record<string, number>} successfulResponseTimeHistogram
  *
  * @typedef {Object} FetchStats
  * @property {number} status http response status
@@ -23,7 +24,7 @@ const GATEWAY_METRICS_ID = 'gateway_metrics'
 /**
  * Durable Object for keeping Metrics state of a gateway.
  */
-export class GatewayMetrics0 {
+export class GatewayMetrics1 {
   constructor(state) {
     this.state = state
 
@@ -85,9 +86,19 @@ export class GatewayMetrics0 {
       this.gatewayMetrics.totalWinnerRequests += 1
     }
 
+    // Only update response time histogram if response is successful
+    if (stats.status === HTTP_STATUS_SUCCESS) {
+      this._updateSuccessfulResponseTimeHistogram(stats)
+    }
+  }
+
+  /**
+   * @param {FetchStats} stats
+   */
+  _updateSuccessfulResponseTimeHistogram(stats) {
     // Update histogram
     const gwHistogram = {
-      ...this.gatewayMetrics.responseTimeHistogram,
+      ...this.gatewayMetrics.successfulResponseTimeHistogram,
     }
 
     // Get all the histogram buckets where the response time is smaller
@@ -98,7 +109,7 @@ export class GatewayMetrics0 {
       gwHistogram[candidate] += 1
     })
 
-    this.gatewayMetrics.responseTimeHistogram = gwHistogram
+    this.gatewayMetrics.successfulResponseTimeHistogram = gwHistogram
   }
 }
 
@@ -109,7 +120,7 @@ function createMetricsTracker() {
     totalWinnerRequests: 0,
     totalResponsesByStatus: {},
     totalRequestsPreventedByReason: {},
-    responseTimeHistogram: createResponseTimeHistogramObject(),
+    successfulResponseTimeHistogram: createResponseTimeHistogramObject(),
   }
 
   return m

packages/edge-gateway/src/index.js

@@ -8,7 +8,7 @@ import { gatewayGet } from './gateway.js'
 import { metricsGet } from './metrics.js'
 
 // Export Durable Object namespace from the root module.
-export { GatewayMetrics0 } from './durable-objects/gateway-metrics.js'
+export { GatewayMetrics1 } from './durable-objects/gateway-metrics.js'
 export { SummaryMetrics1 } from './durable-objects/summary-metrics.js'
 export { CidsTracker0 } from './durable-objects/cids.js'
 export { GatewayRedirectCounter0 } from './durable-objects/gateway-redirect-counter.js'

packages/edge-gateway/src/metrics.js

@@ -233,16 +233,18 @@ export async function metricsGet(request, env, ctx) {
       (gw) =>
         `nftgateway_winner_requests_total{gateway="${gw}",env="${env.ENV}"} ${metricsCollected.ipfsGateways[gw].totalWinnerRequests}`
     ),
-    `# HELP nftgateway_requests_per_time_total total of requests per response time bucket`,
+    `# HELP nftgateway_requests_per_time_total total of successful requests per response time bucket`,
     `# TYPE nftgateway_requests_per_time_total histogram`,
     ...responseTimeHistogram.map((t) => {
       return env.ipfsGateways
         .map(
           (gw) =>
-            `nftgateway_requests_per_time_total{gateway="${gw}",le="${msToS(
+            `nftgateway_successful_requests_per_time_total{gateway="${gw}",le="${msToS(
               t
             )}",env="${env.ENV}"} ${
-              metricsCollected.ipfsGateways[gw].responseTimeHistogram[t]
+              metricsCollected.ipfsGateways[gw].successfulResponseTimeHistogram[
+                t
+              ]
             }`
         )
         .join('\n')

packages/edge-gateway/test/metrics.spec.js

@@ -195,4 +195,11 @@ test('Counts failures', async (t) => {
     ),
     true
   )
+
+  t.is(
+    metricsResponse.includes(
+      `_requests_per_time_total{gateway="${gateways[2]}",le="+Inf",env="test"} 0`
+    ),
+    true
+  )
 })

packages/edge-gateway/wrangler.toml

@@ -18,7 +18,7 @@ main = "worker.mjs"
 
 [durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -41,7 +41,7 @@ ENV = "production"
 
 [env.production.durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -64,7 +64,7 @@ ENV = "staging"
 
 [env.staging.durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -83,7 +83,7 @@ ENV = "test"
 
 [env.test.durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -101,7 +101,7 @@ IPFS_GATEWAYS = "[\"https://ipfs.io\"]"
 
 [env.vsantos.durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -118,7 +118,7 @@ IPFS_GATEWAYS = "[\"https://ipfs.io\"]"
 
 [env.alanshaw.durable_objects]
 bindings = [
-  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics0"},
+  {name = "GATEWAYMETRICS", class_name = "GatewayMetrics1"},
   {name = "SUMMARYMETRICS", class_name = "SummaryMetrics1"},
   {name = "CIDSTRACKER", class_name = "CidsTracker0"},
   {name = "GATEWAYREDIRECTCOUNTER", class_name = "GatewayRedirectCounter0"}
@@ -151,4 +151,8 @@ deleted_classes = ["GatewayRateLimits4"]
 [[migrations]]
 tag = "v7" # Should be unique for each entry
 new_classes = ["SummaryMetrics1"]
-deleted_classes = ["SummaryMetrics0"]
+deleted_classes = ["SummaryMetrics0"]
+[[migrations]]
+tag = "v8" # Should be unique for each entry
+new_classes = ["GatewayMetrics1"]
+deleted_classes = ["GatewayMetrics0"]