Skip to content

Commit

Permalink
BB-473 Probe response logic should be handled in the handler
Browse files Browse the repository at this point in the history 
Currently, the probe response logic is distributed between Backbeat probe handlers and Arsenal's onRequest method.
This scattered approach causes confusion for developers and results in bugs.
The solution is to centralize the probe response logic exclusively within the Backbeat probe handlers.
  • Loading branch information
@nicolas2bert
nicolas2bert committed Dec 1, 2023
1 parent 22408bd commit 7b07abb
Show file tree
Hide file tree
Showing 9 changed files with 496 additions and 95 deletions.
11 changes: 6 additions & 5 deletions extensions/replication/queueProcessor/QueueProcessor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const ObjectQueueEntry = require('../utils/ObjectQueueEntry');
const BucketQueueEntry = require('../utils/BucketQueueEntry');
const constants = require('../../../lib/constants');
const { wrapCounterInc, wrapGaugeSet, wrapHistogramObserve } = require('../../../lib/util/metrics');
const { sendSuccess, sendMultipleErrors } = require('../../../lib/util/probe');

const {
proxyVaultPath,
Expand Down Expand Up @@ -501,7 +502,7 @@ class QueueProcessor extends EventEmitter {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @returns {undefined}
*/
handleLiveness(res, log) {
const verboseLiveness = {};
Expand Down Expand Up @@ -547,11 +548,11 @@ class QueueProcessor extends EventEmitter {
log.debug('verbose liveness', verboseLiveness);

if (responses.length > 0) {
return JSON.stringify(responses);
sendMultipleErrors(res, log, responses);
return undefined;
}

res.writeHead(200);
res.end();
sendSuccess(res, log);
return undefined;
}

Expand All @@ -560,7 +561,7 @@ class QueueProcessor extends EventEmitter {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @return {undefined}
*/
async handleMetrics(res, log) {
log.debug('metrics requested');
Expand Down
11 changes: 6 additions & 5 deletions extensions/replication/replicationStatusProcessor/ReplicationStatusProcessor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const ObjectQueueEntry = require('../utils/ObjectQueueEntry');
const FailedCRRProducer = require('../failedCRR/FailedCRRProducer');
const ReplayProducer = require('../replay/ReplayProducer');
const promClient = require('prom-client');
const { sendSuccess, sendMultipleErrors } = require('../../../lib/util/probe');
const constants = require('../../../lib/constants');
const {
wrapCounterInc,
Expand Down Expand Up @@ -395,7 +396,7 @@ class ReplicationStatusProcessor {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @returns {undefined}
*/
handleLiveness(res, log) {
const verboseLiveness = {};
Expand Down Expand Up @@ -430,11 +431,11 @@ class ReplicationStatusProcessor {
log.debug('verbose liveness', verboseLiveness);

if (responses.length > 0) {
return JSON.stringify(responses);
sendMultipleErrors(res, log, responses);
return undefined;
}

res.writeHead(200);
res.end();
sendSuccess(res, log);
return undefined;
}

Expand All @@ -443,7 +444,7 @@ class ReplicationStatusProcessor {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @return {undefined}
*/
async handleMetrics(res, log) {
log.debug('metrics requested');
Expand Down
11 changes: 6 additions & 5 deletions lib/queuePopulator/QueuePopulator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const FailedCRRConsumer =
const NotificationConfigManager
= require('../../extensions/notification/NotificationConfigManager');
const promClient = require('prom-client');
const { sendSuccess, sendMultipleErrors } = require('../util/probe');
const constants = require('../constants');
const { wrapCounterInc, wrapGaugeSet } = require('../util/metrics');

Expand Down Expand Up @@ -419,7 +420,7 @@ class QueuePopulator {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @returns {undefined}
*/
handleLiveness(res, log) {
// log verbose status for all checks
Expand Down Expand Up @@ -453,11 +454,11 @@ class QueuePopulator {
log.debug('verbose liveness', verboseLiveness);

if (responses.length > 0) {
return JSON.stringify(responses);
sendMultipleErrors(res, log, responses);
return undefined;
}

res.writeHead(200);
res.end();
sendSuccess(res, log);
return undefined;
}

Expand All @@ -466,7 +467,7 @@ class QueuePopulator {
*
* @param {http.HTTPServerResponse} res - HTTP Response to respond with
* @param {Logger} log - Logger
* @returns {string} Error response string or undefined
* @return {undefined}
*/
async handleMetrics(res, log) {
log.debug('metrics requested');
Expand Down
62 changes: 48 additions & 14 deletions lib/util/probe.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,30 +32,64 @@ function startProbeServer(config, callback) {
probeServer.start();
}

/**
* Send an Error response with multiple errors
* @param {http.HTTPServerResponse} res - HTTP response for writing
* @param {Logger} log - Werelogs instance for logging if you choose to
* @param {Object} errorMessages - error messages
* @return {undefined}
*/
function sendMultipleErrors(res, log, errorMessages) {
const messages = JSON.stringify(errorMessages);
const errorCode = 500;
log.error('sending back error response', {
httpCode: errorCode,
error: messages,
});
res.writeHead(errorCode);
res.end(messages);
}

/**
* Send an Error response
* @param {http.HTTPServerResponse} res - HTTP response for writing
* @param {Logger} log - Werelogs instance for logging if you choose to
* @param {Error} error - Error to send back to the user
* @param {String} [optMessage] - Message to use instead of the errors message
* @return {undefined}
*/
function sendError(res, log, error, optMessage) {
res.writeHead(error.code);
let message;
if (optMessage) {
message = optMessage;
} else {
message = error.description || '';
}
log.debug('sending back error response', { httpCode: error.code,
const message = optMessage || error.description || '';
log.error('sending back error response', {
httpCode: error.code,
errorType: error.message,
error: message });
res.end(`${JSON.stringify({ errorType: error.message,
errorMessage: message })}\n`);
error: message,
});
res.writeHead(error.code);
res.end(
JSON.stringify({
errorType: error.message,
errorMessage: message,
}),
);
}

function sendSuccess(res, log, msg) {
res.writeHead(200);
/**
* Send a successful HTTP response of 200 OK
* @param {http.HTTPServerResponse} res - HTTP response for writing
* @param {Logger} log - Werelogs instance for logging if you choose to
* @param {String} [message] - Message to send as response, defaults to OK
* @return {undefined}
*/
function sendSuccess(res, log, message = 'OK') {
log.debug('replying with success');
const message = msg || 'OK';
res.writeHead(200);
res.end(message);
}

module.exports = {
startProbeServer,
sendSuccess,
sendError,
sendMultipleErrors,
};
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"homepage": "https://github.com/scality/backbeat#readme",
"dependencies": {
"@hapi/joi": "^15.1.0",
"arsenal": "git+https://github.com/scality/arsenal#7.10.46",
"arsenal": "git+https://github.com/scality/arsenal#7.10.50",
"async": "^2.3.0",
"aws-sdk": "^2.1326.0",
"backo": "^1.1.0",
Expand Down
101 changes: 79 additions & 22 deletions tests/unit/QueuePopulator.spec.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
const assert = require('assert');
const promClient = require('prom-client');

const { Logger } = require('werelogs');
const sinon = require('sinon');
Expand Down Expand Up @@ -75,6 +76,7 @@ describe('QueuePopulator', () => {
mockRes = sinon.spy();
mockLog = sinon.spy();
mockLog.debug = sinon.spy();
mockLog.error = sinon.spy();
mockRes.writeHead = sinon.spy();
mockRes.end = sinon.spy();
});
Expand All @@ -89,10 +91,18 @@ describe('QueuePopulator', () => {
};
const response = qp.handleLiveness(mockRes, mockLog);
assert.strictEqual(response, undefined);
sinon.assert.calledOnceWithExactly(
mockLog.debug,
sinon.match.any, // we don't care about the debug label
{ zookeeper: zookeeper.State.SYNC_CONNECTED.code }

sinon.assert.calledTwice(mockLog.debug);
// First call assertion
sinon.assert.calledWith(
mockLog.debug.getCall(0), // getCall(0) retrieves the first call
'verbose liveness',
{ zookeeper: zookeeper.State.SYNC_CONNECTED.code },
);
// Second call assertion
sinon.assert.calledWith(
mockLog.debug.getCall(1), // getCall(1) retrieves the second call
'replying with success',
);
sinon.assert.calledOnceWithExactly(mockRes.writeHead, 200);
sinon.assert.calledOnce(mockRes.end);
Expand All @@ -117,14 +127,21 @@ describe('QueuePopulator', () => {
};
const response = qp.handleLiveness(mockRes, mockLog);
assert.strictEqual(response, undefined);
sinon.assert.calledOnceWithExactly(
mockLog.debug,
sinon.match.any, // we don't care about the debug label
sinon.assert.calledTwice(mockLog.debug);
// First call assertion
sinon.assert.calledWith(
mockLog.debug.getCall(0), // getCall(0) retrieves the first call
'verbose liveness',
{
'zookeeper': zookeeper.State.SYNC_CONNECTED.code,
'producer-topicA': true,
'producer-topicB': true,
}
},
);
// Second call assertion
sinon.assert.calledWith(
mockLog.debug.getCall(1), // getCall(1) retrieves the second call
'replying with success',
);
sinon.assert.calledOnceWithExactly(mockRes.writeHead, 200);
sinon.assert.calledOnce(mockRes.end);
Expand All @@ -148,27 +165,67 @@ describe('QueuePopulator', () => {
getState: () => zookeeper.State.SYNC_CONNECTED,
};
const response = qp.handleLiveness(mockRes, mockLog);
assert.deepStrictEqual(
JSON.parse(response),
[
{
component: 'log reader',
status: constants.statusNotReady,
topic: 'topicB',
},
]
);
assert.strictEqual(response, undefined);
sinon.assert.calledOnceWithExactly(
mockLog.debug,
sinon.match.any, // we don't care about the debug label
'verbose liveness', // we don't care about the debug label
{
'zookeeper': zookeeper.State.SYNC_CONNECTED.code,
'producer-topicA': true,
'producer-topicB': false,
}
},
);
sinon.assert.calledOnceWithExactly(mockRes.writeHead, 500);
const expectedRes = JSON.stringify([
{
component: 'log reader',
status: constants.statusNotReady,
topic: 'topicB',
},
]);
sinon.assert.calledOnceWithExactly(
mockLog.error,
'sending back error response', // we don't care about the debug label
{
httpCode: 500,
error: expectedRes,
},
);
sinon.assert.notCalled(mockRes.writeHead);
sinon.assert.notCalled(mockRes.end);
sinon.assert.calledOnceWithExactly(mockRes.end, expectedRes);
});
});

describe('handle metrics', () => {
let response;
let logger;

beforeEach(() => {
response = {
writeHead: sinon.stub(),
end: sinon.stub(),
};
logger = {
debug: sinon.stub(),
error: sinon.stub(),
};
sinon.stub(promClient.register, 'metrics').resolves('metrics_data');
});

it('should handle metrics correctly', async () => {
const r = await qp.handleMetrics(response, logger);
assert.strictEqual(r, undefined);

sinon.assert.calledOnce(response.writeHead);
sinon.assert.calledOnceWithExactly(response.writeHead, 200, {
'Content-Type': promClient.register.contentType,
});

sinon.assert.calledOnce(response.end);
sinon.assert.calledWithExactly(response.end, 'metrics_data');

sinon.assert.calledOnce(logger.debug);
sinon.assert.calledWithExactly(logger.debug, 'metrics requested');
return;
});
});
});
Loading

0 comments on commit 7b07abb

Please sign in to comment.