Skip to main content
4 of 7
added 308 characters in body
Mike Brant
  • 9.6k
  • 13
  • 23

Some thoughts:

  • You are using async but not "using" it. You can simplify this for the reader (and often subsequent code) by doing const accessToken = await axios.post(...); and similar to de-nest and make the code more understandable. This also helps your avoid bad variable names like res1, res2, res3, etc.
  • Promise.all() is your friend when it comes to making batch calls like this. Combined with a tool like your waitFor function, you can introduce any kind of request pattern you want. Right now you are making calls in series which is kind of harder to throttle when it comes to meeting rate limits, as not you are concerned about accounting for response times in determine when to make next call vs. an up-front.
  • If you are going to use a tool like axios or Fetch API that require a res.data or res.json() type of step, you might consider putting a simple wrapper around the response handling to clean up code a bit. This is also a good way to standardize error handling at the client level.
  • Break down your code along logical functional lines (another option available to you after using async more conventionally). For example, the status request and strapi update are one logical operational unit.
  • If you control your strapi endpoint/database you should consider adding bulk query support for the datapoint updates. Querying in loops is kind of an anti-pattern and pretty much any modern data store supports bulk updates.
  • I would recommend defining key constants/variables for aiding in code understanding and maintainence. For example, if I wanted to migrate to v2 of this API at some point, I don't want to have to go replace v1 with v2 in a bunch of places. Centralize how you hold such module "configuration" logic.
  • Consider implementing more appropriate error handling. My guess is that this server monitoring process shouldn't be designed to just silently swallow errors.

Putting it together might yield something like:

import axios from 'axios';
import strapi from 'your-strapi-module';

const apiBase = `https://api.cloudways.com/api/v1`;
const apiKey = process.env.CLOUDWAYS_API;
const email = process.env.CLOUDWAYS_EMAIL;
const maxRps = 10; // max requests per second for throttling
const throttleInterval = 1000 / maxRps;

// a small wrapper around axios for this particular API
// this might best live in a separate module
const apiRequest = async ({ headers = {}, method = 'get', url }) => {
  const opts = {
    baseUrl: apiBase,
    headers,
    method,
    url
  };
  return axios(opts).then( (res) => res.data );
};

// method to obtain OAuth token, again perhaps best in separate module with API axios wrapper.
// this could be made to lazily provision the token on any API request if not already received
const getAccessToken = async () => {
  const method = 'post';
  const url = `/oauth/access_token?email=${ email }&api_key=${ apiKey }`;

  const data = await apiRequest({ method, url});
  return data.access_token;
};

const getServerList = async () => strapi.query('server').find({});

const throttle = async (asyncFn, ms) => {
  await new Promise( (resolve) => setTimeout(resolve, ms) );
  return asyncFn();
};

const updateServerStatus = async (serverId, accessToken) => {
  const url = `/server/monitor/summary?server_id=${ serverId }&type=db`;
  const headers = {
    Authorization: 'Bearer ' + accessToken
  };

  return apiRequest({ headers, url })
    .then( ({ content }) => {
      // I am not throttling here as no need to throttle this data store query was noted
      // Ideally, this could be made into single update query though
      const promises = content.map( (app) => {
        return strapi.query('subscription').update(
          { ApplicationSlug: app.name },
          { DiskUsage: Math.ceil(app.datapoint[0]) }
        );
      });

      return Promise.all(promises);
    });
};

const updateAll = async () = {
  // we can parallelize getting access token and getting server list
  const initPromises = [
    getServerList(),
    getAccessToken()
  ];

  const [ serverList, accessToken ] = await Promise.all(initPromises);

  // map your server list to an array of update promises
  const updatePromises = serverList.map((server, i) => {
    const fn = () => updateServerStatus(server.ServerId, accessToken);

    // here is how we stagger the requests to meet rate limits
    const wait = i * throttleInterval;
    return throttle(fn, wait);
  });

  return Promise.all(updatePromises);
};

// call the main function somehow (could be exported from module for example)
export {
  updateAll as default,
  updateAll,
  // again, following probably make sense in separate module
  apiRequest,
  getAccessToken
}

Note that since we are now "using" async in a more appropriate fashion you now begin to make your logic available in specific functions. For example, if I just wanted to allow one to update the status of a single server, I could expose the updateServerStatus function in exports. There would have been no way to offer that sort of code flexibility with your current approach of using nested thenables.

Mike Brant
  • 9.6k
  • 13
  • 23