From 25353708428006105f87aae46d8c57fa2312eb68 Mon Sep 17 00:00:00 2001 From: Thomas Flament Date: Wed, 12 Aug 2026 15:33:48 +0200 Subject: [PATCH 1/2] Prettier reformat monitoringHandler and its tests Issue: CLDSRV-971 --- lib/utilities/monitoringHandler.js | 114 +++++++++++++++-------------- tests/unit/utils/monitoring.js | 49 +++++++++---- 2 files changed, 93 insertions(+), 70 deletions(-) diff --git a/lib/utilities/monitoringHandler.js b/lib/utilities/monitoringHandler.js index 712a083067..d6b79633da 100644 --- a/lib/utilities/monitoringHandler.js +++ b/lib/utilities/monitoringHandler.js @@ -114,69 +114,72 @@ if (config.isQuotaEnabled) { // labels and buckets. const lifecycleDuration = new client.Histogram({ name: 's3_lifecycle_duration_seconds', - help: 'Duration of the lifecycle operation, calculated from the theoretical date to the end ' + - 'of the operation', + help: 'Duration of the lifecycle operation, calculated from the theoretical date to the end ' + 'of the operation', labelNames: ['type', 'location'], buckets: [0.2, 1, 5, 30, 120, 600, 3600, 4 * 3600, 8 * 3600, 16 * 3600, 24 * 3600], }); -function promMetrics(method, bucketName, code, action, - newByteLength, oldByteLength, isVersionedObj, - numOfObjectsRemoved, ingestSize) { +function promMetrics( + method, + bucketName, + code, + action, + newByteLength, + oldByteLength, + isVersionedObj, + numOfObjectsRemoved, + ingestSize, +) { let bytes; switch (action) { - case 'putObject': - case 'copyObject': - case 'putObjectPart': - if (code === '200') { - bytes = newByteLength - (isVersionedObj ? 0 : oldByteLength); - httpRequestSizeBytes - .labels(method, action, code) - .observe(newByteLength); - dataDiskAvailable.dec(bytes); - dataDiskFree.dec(bytes); - if (ingestSize) { - numberOfIngestedObjects.inc(); - dataIngested.inc(ingestSize); + case 'putObject': + case 'copyObject': + case 'putObjectPart': + if (code === '200') { + bytes = newByteLength - (isVersionedObj ? 0 : oldByteLength); + httpRequestSizeBytes.labels(method, action, code).observe(newByteLength); + dataDiskAvailable.dec(bytes); + dataDiskFree.dec(bytes); + if (ingestSize) { + numberOfIngestedObjects.inc(); + dataIngested.inc(ingestSize); + } + numberOfObjects.inc(); } - numberOfObjects.inc(); - } - break; - case 'createBucket': - if (code === '200') { - numberOfBuckets.inc(); - } - break; - case 'getObject': - if (code === '200') { - httpResponseSizeBytes - .labels(method, action, code) - .observe(newByteLength); - } - break; - case 'deleteBucket': - case 'deleteBucketWebsite': - if (code === '200' || code === '204') { - numberOfBuckets.dec(); - } - break; - case 'deleteObject': - case 'abortMultipartUpload': - case 'multiObjectDelete': - if (code === '200') { - dataDiskAvailable.inc(newByteLength); - dataDiskFree.inc(newByteLength); - const objs = numOfObjectsRemoved || 1; - numberOfObjects.dec(objs); - if (ingestSize) { - numberOfIngestedObjects.dec(objs); - dataIngested.dec(ingestSize); + break; + case 'createBucket': + if (code === '200') { + numberOfBuckets.inc(); } - } - break; - default: - break; + break; + case 'getObject': + if (code === '200') { + httpResponseSizeBytes.labels(method, action, code).observe(newByteLength); + } + break; + case 'deleteBucket': + case 'deleteBucketWebsite': + if (code === '200' || code === '204') { + numberOfBuckets.dec(); + } + break; + case 'deleteObject': + case 'abortMultipartUpload': + case 'multiObjectDelete': + if (code === '200') { + dataDiskAvailable.inc(newByteLength); + dataDiskFree.inc(newByteLength); + const objs = numOfObjectsRemoved || 1; + numberOfObjects.dec(objs); + if (ingestSize) { + numberOfIngestedObjects.dec(objs); + dataIngested.dec(ingestSize); + } + } + break; + default: + break; } } @@ -216,8 +219,7 @@ function writeResponse(res, error, results, cb) { } const registry = config.isCluster ? new client.AggregatorRegistry() : client.register; -const getMetrics = config.isCluster ? - registry.clusterMetrics.bind(registry) : registry.metrics.bind(registry); +const getMetrics = config.isCluster ? registry.clusterMetrics.bind(registry) : registry.metrics.bind(registry); async function routeHandler(req, res, cb) { if (req.method !== 'GET') { diff --git a/tests/unit/utils/monitoring.js b/tests/unit/utils/monitoring.js index 96ef99070b..3a2ed260c5 100644 --- a/tests/unit/utils/monitoring.js +++ b/tests/unit/utils/monitoring.js @@ -7,8 +7,12 @@ const monitoring = require('../../../lib/utilities/monitoringHandler'); describe('Monitoring: endpoint', () => { const sandbox = sinon.createSandbox(); const res = { - writeHead(/* result, headers */) { return this; }, - write(/* body */) { return this; }, + writeHead(/* result, headers */) { + return this; + }, + write(/* body */) { + return this; + }, end(/* body */) {}, }; monitoring.collectDefaultMetrics(); @@ -23,9 +27,20 @@ describe('Monitoring: endpoint', () => { }); async function fetchMetrics(req, res) { - await new Promise(resolve => monitoring.monitoringHandler(null, req, { - ...res, end: (...body) => { res.end(...body); resolve(); } - }, null)); + await new Promise(resolve => + monitoring.monitoringHandler( + null, + req, + { + ...res, + end: (...body) => { + res.end(...body); + resolve(); + }, + }, + null, + ), + ); } it('should return an error is method is not GET', async () => { @@ -80,20 +95,28 @@ describe('Monitoring: endpoint', () => { }); function parseMetric(metrics, name, labels) { - const labelsString = Object.entries(labels).map(e => `${e[0]}="${e[1]}"`).join(','); + const labelsString = Object.entries(labels) + .map(e => `${e[0]}="${e[1]}"`) + .join(','); const metric = metrics.match(new RegExp(`^${name}{${labelsString}} (.*)$`, 'm')); return metric ? metric[1] : null; } function parseHttpRequestSize(metrics, action = 'putObject') { - const value = parseMetric(metrics, 's3_cloudserver_http_request_size_bytes_sum', - { method: 'PUT', action, code: '200' }); + const value = parseMetric(metrics, 's3_cloudserver_http_request_size_bytes_sum', { + method: 'PUT', + action, + code: '200', + }); return value ? parseInt(value, 10) : 0; } function parseHttpResponseSize(metrics, action = 'getObject') { - const value = parseMetric(metrics, 's3_cloudserver_http_response_size_bytes_sum', - { method: 'GET', action, code: '200' }); + const value = parseMetric(metrics, 's3_cloudserver_http_response_size_bytes_sum', { + method: 'GET', + action, + code: '200', + }); return value ? parseInt(value, 10) : 0; } @@ -101,8 +124,7 @@ describe('Monitoring: endpoint', () => { await fetchMetrics({ method: 'GET', url: '/metrics' }, res); const requestSize = parseHttpRequestSize(res.end.args[0][0]); - monitoring.promMetrics('PUT', 'stuff', '200', - 'putObject', 2357, 3572, false, null, 5723); + monitoring.promMetrics('PUT', 'stuff', '200', 'putObject', 2357, 3572, false, null, 5723); await fetchMetrics({ method: 'GET', url: '/metrics' }, res); assert(parseHttpRequestSize(res.end.args[1][0]) === requestSize + 2357); @@ -112,8 +134,7 @@ describe('Monitoring: endpoint', () => { await fetchMetrics({ method: 'GET', url: '/metrics' }, res); const responseSize = parseHttpResponseSize(res.end.args[0][0]); - monitoring.promMetrics('GET', 'stuff', '200', - 'getObject', 7532); + monitoring.promMetrics('GET', 'stuff', '200', 'getObject', 7532); await fetchMetrics({ method: 'GET', url: '/metrics' }, res); assert(parseHttpResponseSize(res.end.args[1][0]) === responseSize + 7532); From eeb84c53953700c4a2214a9a0f537c3efcef1060 Mon Sep 17 00:00:00 2001 From: Thomas Flament Date: Wed, 12 Aug 2026 15:57:07 +0200 Subject: [PATCH 2/2] Register quota metrics unconditionally Both quota gates in monitoringHandler tested config.isQuotaEnabled -- the function reference rather than a call -- so both were always true and all six quota metrics were always registered. Drop the conditionals rather than calling the function. Always registering is what has actually happened since these checks were written, nothing depends on the metrics disappearing when quota is off, and gating registration would mean guarding every metric touch point against a metric that was never created: crrCacheToProm reached bucketsWithQuota unguarded, and the scuba wrapper observes its histogram in a finally block, on a path the Veeam capacity route hits with no quota gating at all. Behaviour is unchanged -- the six s3_cloudserver_quota_* series are exposed exactly as before -- so this is a dead code removal. Issue: CLDSRV-972 --- lib/utilities/monitoringHandler.js | 71 +++++++++++++----------------- tests/unit/utils/monitoring.js | 32 ++++++++++++++ 2 files changed, 63 insertions(+), 40 deletions(-) diff --git a/lib/utilities/monitoringHandler.js b/lib/utilities/monitoringHandler.js index d6b79633da..be889935bc 100644 --- a/lib/utilities/monitoringHandler.js +++ b/lib/utilities/monitoringHandler.js @@ -65,48 +65,39 @@ const httpResponseSizeBytes = new client.Summary({ help: 'Cloudserver HTTP response sizes in bytes', }); -let quotaEvaluationDuration; -let utilizationMetricsRetrievalDuration; -let utilizationServiceAvailable; -let bucketsWithQuota; -let accountsWithQuota; -let requestWithQuotaMetricsUnavailable; - -if (config.isQuotaEnabled) { - quotaEvaluationDuration = new client.Histogram({ - name: 's3_cloudserver_quota_evaluation_duration_seconds', - help: 'Duration of the quota evaluation operation', - labelNames: ['action', 'code', 'type'], - buckets: [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.2, 0.5, 1], - }); +const quotaEvaluationDuration = new client.Histogram({ + name: 's3_cloudserver_quota_evaluation_duration_seconds', + help: 'Duration of the quota evaluation operation', + labelNames: ['action', 'code', 'type'], + buckets: [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.2, 0.5, 1], +}); - utilizationMetricsRetrievalDuration = new client.Histogram({ - name: 's3_cloudserver_quota_metrics_retrieval_duration_seconds', - help: 'Duration of the utilization metrics retrieval operation', - labelNames: ['code', 'class'], - buckets: [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.2, 0.5], - }); +const utilizationMetricsRetrievalDuration = new client.Histogram({ + name: 's3_cloudserver_quota_metrics_retrieval_duration_seconds', + help: 'Duration of the utilization metrics retrieval operation', + labelNames: ['code', 'class'], + buckets: [0.001, 0.0025, 0.005, 0.01, 0.025, 0.05, 0.1, 0.2, 0.5], +}); - utilizationServiceAvailable = new client.Gauge({ - name: 's3_cloudserver_quota_utilization_service_available', - help: 'Availability of the utilization service', - }); +const utilizationServiceAvailable = new client.Gauge({ + name: 's3_cloudserver_quota_utilization_service_available', + help: 'Availability of the utilization service', +}); - bucketsWithQuota = new client.Gauge({ - name: 's3_cloudserver_quota_buckets_count', - help: 'Total number of buckets quota', - }); +const bucketsWithQuota = new client.Gauge({ + name: 's3_cloudserver_quota_buckets_count', + help: 'Total number of buckets quota', +}); - accountsWithQuota = new client.Gauge({ - name: 's3_cloudserver_quota_accounts_count', - help: 'Total number of account quota', - }); +const accountsWithQuota = new client.Gauge({ + name: 's3_cloudserver_quota_accounts_count', + help: 'Total number of account quota', +}); - requestWithQuotaMetricsUnavailable = new client.Counter({ - name: 's3_cloudserver_quota_unavailable_count', - help: 'Total number of requests with quota metrics unavailable', - }); -} +const requestWithQuotaMetricsUnavailable = new client.Counter({ + name: 's3_cloudserver_quota_unavailable_count', + help: 'Total number of requests with quota metrics unavailable', +}); // Lifecycle duration metric, to track the completion of restore. // This metric is used to track the time it takes to complete the lifecycle operation (restore). @@ -189,10 +180,10 @@ function crrCacheToProm(crrResults) { if (crrResults.getObjectCount) { numberOfBuckets.set(crrResults.getObjectCount.buckets || 0); numberOfObjects.set(crrResults.getObjectCount.objects || 0); + bucketsWithQuota.set(crrResults.getObjectCount.bucketWithQuotaCount || 0); } - if (config.isQuotaEnabled) { - bucketsWithQuota.set(crrResults?.getObjectCount?.bucketWithQuotaCount || 0); - accountsWithQuota.set(crrResults?.getVaultReport?.accountWithQuotaCount || 0); + if (crrResults.getVaultReport) { + accountsWithQuota.set(crrResults.getVaultReport.accountWithQuotaCount || 0); } if (crrResults.getDataDiskUsage) { dataDiskAvailable.set(crrResults.getDataDiskUsage.available || 0); diff --git a/tests/unit/utils/monitoring.js b/tests/unit/utils/monitoring.js index 3a2ed260c5..111fb40fec 100644 --- a/tests/unit/utils/monitoring.js +++ b/tests/unit/utils/monitoring.js @@ -3,6 +3,7 @@ const promclient = require('prom-client'); const sinon = require('sinon'); const monitoring = require('../../../lib/utilities/monitoringHandler'); +const { config } = require('../../../lib/Config'); describe('Monitoring: endpoint', () => { const sandbox = sinon.createSandbox(); @@ -139,4 +140,35 @@ describe('Monitoring: endpoint', () => { await fetchMetrics({ method: 'GET', url: '/metrics' }, res); assert(parseHttpResponseSize(res.end.args[1][0]) === responseSize + 7532); }); + + const quotaMetricNames = [ + 's3_cloudserver_quota_evaluation_duration_seconds', + 's3_cloudserver_quota_metrics_retrieval_duration_seconds', + 's3_cloudserver_quota_utilization_service_available', + 's3_cloudserver_quota_buckets_count', + 's3_cloudserver_quota_accounts_count', + 's3_cloudserver_quota_unavailable_count', + ]; + + it('should register quota metrics even when quota is disabled', async () => { + assert.strictEqual(config.isQuotaEnabled(), false); + + await fetchMetrics({ method: 'GET', url: '/metrics' }, res); + const metrics = res.end.args[0][0]; + + quotaMetricNames.forEach(name => assert(metrics.includes(name), `${name} is not registered`)); + }); + + it('should report the quota counts from crrCacheToProm', async () => { + monitoring.crrCacheToProm({ + getObjectCount: { buckets: 1, objects: 2, bucketWithQuotaCount: 3 }, + getVaultReport: { accountWithQuotaCount: 4 }, + }); + + await fetchMetrics({ method: 'GET', url: '/metrics' }, res); + const metrics = res.end.args[0][0]; + + assert(metrics.includes('\ns3_cloudserver_quota_buckets_count 3')); + assert(metrics.includes('\ns3_cloudserver_quota_accounts_count 4')); + }); });