Node.js package that retrieves data from an API, formats it and sends it out via email to a distribution list

Question

I've created a Node.js package that retrieves data from Icinga (a monitoring platform), formats it and passes it off to a class that generates some HTML and then sends it all out as an email.

The email, in its simplest form, looks something like this:

Basic image of said table

I come from a Ruby/Python background and this is the first time I've ever delved into Node. The code below works and does what I need it to but I have the feeling it isn't using some of Node's best practices. I make 2 API calls, after the first one happens, inside the callback, I make another API call and then when all the data is returned I invoke a function to send off the email. I'm pretty sure this could be using async/await and/or Promises but I'm just not sure where to start in order to refactor it.

// Get the data
const warning = 1;
const error = 2;
const icingaServer = new icingaApi(icingaConfig.url, icingaConfig.port, icingaConfig.username, icingaConfig.password);
const clients = [
 { 'Client 1': '**.**.client_1.**.**' },
 { 'Client 2': '**.**.client_2.**.**' }
 ];
const data = [];
function sendEmail() {
 nodemailerMailgun.sendMail({
 from: '[email protected]',
 to: appConfig.sendees,
 subject: 'Some subject',
 html: new tableHtmlGenerator(data).run()
 }).then((_res) => {
 let emailAddresses = appConfig.sendees.join(', ');
 console.log(`Email sent successfully to the following addresses: ${emailAddresses}`);
 }).catch((err) => {
 console.log(`Error: ${err.message}`);
 });
}
function allDataRetrieved() {
 return data.length === clients.length;
}
clients.forEach((clientMap) => {
 Object.entries(clientMap).forEach(([client, hostnameWildcard]) => {
 let totalHosts;
 let totalServices;
 let errors;
 let warnings;
 icingaServer.getServiceFiltered({
 "filter": "match(service_name, service.host_name)",
 "filter_vars": {
 "service_name": hostnameWildcard
 }
 }, (err, res) => {
 if (err) return `Error: ${err}`;
 warnings = res.filter(o => o.attrs.state === warning).length;
 errors = res.filter(o => o.attrs.state === error).length;
 totalServices = res.length;
 icingaServer.getHostFiltered({
 "filter": "match(host_name, host.name)",
 "filter_vars": {
 "host_name": hostnameWildcard
 }
 }, (err, res) => {
 if (err) return `Error: ${err}`;
 warnings += res.filter(o => o.attrs.state === warning).length;
 errors += res.filter(o => o.attrs.state === error).length;
 totalHosts = res.length;
 data.push({
 name: `${client} (${totalHosts}/${totalServices})`,
 errors: errors,
 warnings: warnings
 });
 if (allDataRetrieved()) sendEmail();
 });
 });
 });
});

I've omitted all the require and const definitions at the top of this file as they're not really needed in order to understand the code in my opinion.

The main issue is that one API call happens inside the callback of another API call and this feels nasty to me. I also wait for all the data to be pushed to the data variable by doing a simple but crude if statement to check if all the data has been retrieved and pushed to the array, if it has then the email is sent.

I also feel like I need to add that I'm aware this code could be improved by dumping all this business logic into a class or splitting it out into separate files. I'm not after help in that sense, it's more of how to handle API requests and waiting for the requests to finish and when/how/if to use Promises.

Answer 1

When you have a lot of asynchronous requests to make, and you want to wait for all to finish, the proper method to use is to first promisify the requests (if they don't return a Promise already), and then to use Promise.all, which returns a Promise which resolves once all promises in the passed array have resolved.

Unfortunately, icingaServer looks to be callback-based, and you have to multiple methods from it. Luckily, there's a package called promisify that makes turning these callback APIs into Promises much easier.

You can reduce the nesting level by one by changing

clients.forEach((clientMap) => {
 Object.entries(clientMap).forEach(([client, hostnameWildcard]) => {

into a use of flatMap instead: clients.flatMap(Object.entries)

DRY You have two very similar callbacks in getServiceFiltered and getHostFiltered. All that differs is the function invoked and the parameter passed, so it'd be better to make another function to which you can pass the parts that change.

Parallel processing Rather than two calls which are made in serial, consider making them both at once, in parallel, if the API supports it - this'll let your script finish quicker.

Filtered array length While you can do arr.filter(callback).length, you might consider using reduce instead, since you don't care about the resulting array, you just care about the number of matching elements.

Unquoted keys In JS, there's no need to quote object keys unless the keys contain characters that are invalid for idenfifiers. Most prefer not to use quotes so as to keep the code free of unnecessary noise.

Refactored:

const { promisify } = require('util');
const icingaServer = new icingaApi(icingaConfig.url, icingaConfig.port, icingaConfig.username, icingaConfig.password);
const getServiceFiltered = promisify(icingaServer.getServiceFiltered).bind(icingaServer);
const getHostFiltered = promisify(icingaServer.getHostFiltered).bind(icingaServer);
const processClient = async ([client, hostnameWildcard]) => {
 const getClientData = async (method, filterKey) => {
 const result = await method({
 filter: 'match(host_name, host.name)',
 filter_vars: {
 [filterKey]: hostnameWildcard
 }
 });
 return {
 warnings: result.reduce((count, o) => count + (o.attrs.state === warning), 0),
 errors: result.reduce((count, o) => count + (o.attrs.state === warning), 0),
 totalCount: result.length,
 };
 };
 const [serviceData, hostData] = await Promise.all([
 getClientData(getServiceFiltered, 'service_name'),
 getClientData(getHostFiltered, 'host_name'),
 ]);
 return {
 name: `${client} (${hostData.totalCount}/${serviceData.totalCount})`,
 errors: serviceData.errors + hostData.errors,
 warnings: serviceData.warnings + hostData.warnings,
 };
};
Promise.all(
 clients.flatMap(Object.entries).map(processClient)
)
 .then(sendEmail)
 .catch((error) => {
 // handle errors
 });

where sendEmail now takes a parameter of the data to send.

Also note that // handle errors shouldn't just log an error if it occurs - ideally, you'd have a system in which such an error results in a developer being able to look at a dashboard or something to see at a glance what's failed recently, to see when an issue arises that needs looking into (for example, if the API changes and all requests start failing).

• Arran Scott: Thanks for an in-depth answer @CertainPerformance. So when .then(sendEmail) is ran, you're passing in the function sendEmail, will the returned data from the promise above be 'magically' available in the sendEmail function itself?
• CertainPerformance: Not magically - see how the answer says where sendEmail now takes a parameter of the data to send. So you'd need function sendEmail(data), and then the .then(sendEmail) will invoke sendEmail with the resolve value of the previous Promise.
• Arran Scott: Ok, great - thanks for your help. Much appreciated!