0

So I have forked a Javascript project and trying to extend it a bit and at the same time learn from it. My Javascript skills are really newbish but I thought it would be fun. I am however really struggling with all the promises to the point that I have some many promises and thenables that I really don't understand how it is situated anymore. I fail to get the end result and are unable to see why.

So I have this to start from (I disabled one function to keep it simple):

export const calculateMovingAverage = (event, context, callback) =>
  Promise.all([
    // ma.calculateMovingAverage('Kraken', 'AskPrice'),
    ma.calculateMovingAverage('Coinbase', 'SellPrice'),
  ])
    .then((tx) => {
      console.log('tx', tx);
    }).catch((err) => {
      console.error('err', err);
      callback({ statusCode: 500, body: { message: 'Something went wrong' } });
    });

Which thus calls ma.calculateMovingAverage():

calculateMovingAverage(exchange, metric) {
    const self = this;
    const args = {
      minutes: 10,
      period: 60,
      currency: `${self.cryptoCurrency}`,
      metricname: `${metric}`,
      localCurrency: `${self.localCurrency}`,
      namespace: `${exchange}`,
    };
    var promisedland = new Promise((resolve, reject) => {
      self.cloudwatch.getMetricStatistics(args, (err, data) => {
        if (err) { return reject(err); }

        if (!Array.isArray(data.Datapoints) || !data.Datapoints.length) { return reject("No Datapoints received from CloudWatch!")}

        data.Datapoints.forEach(function(item) {
          self.ma.push(item.Timestamp, item.Average);
        });

        resolve(ma.movingAverage());
      })
    })
    promisedland.then((results) => {
      return new Promise((resolve, reject) => {
        const body = {
          value: results,
          metricName: `${metric} @ 180 MovingAverage`,
          namespace: `${exchange}`
        };
        self.cloudwatch.putAverageMetricData(body, function(err, result) {
          if (err) { return reject(err); }
          resolve(result);
        });
      }
    )
    }).catch(function(err) {
      return reject(err);
    });
  }

Now as you can see inside calculateMovingAverage() I try to call two AWS methods. getMetricStatistics and putAverageMetricData.

The first one, the getMetricStatistics functions works beautifully as I get back the Datapoints nicely from AWS.

The function itself:

  getMetricStatistics(args) {
    return this.cloudwatch.getMetricStatistics({
      EndTime: moment().subtract(180, 'minutes').utc().format(),
      Dimensions: [
        {
          Name: 'CryptoCurrency',
          Value: args.currency
        },
        {
          Name: 'LocalCurrency',
          Value: args.localCurrency
        },
        {
          Name: 'Stage',
          Value: process.env.STAGE || 'dev'
        }
      ],
      MetricName: args.metricname,
      Period: Number(args.period),
      StartTime: moment().subtract(190, 'minutes').utc().format(),
      Statistics: ['Average'],
      Unit: 'Count',
      Namespace: args.namespace || 'Coinboss',
    }).promise();
  }

Next I want to pass on the response through the MovingAverage module and would like to put the outcome of MA into CloudWatch Metrics via the putAverageMetricData function:

putAverageMetricData(args) {
    return this.cloudwatch.putMetricData({
      MetricData: [
        {
          MetricName: args.metricName,
          Timestamp: moment().utc().format(),
          Unit: 'Count',
          Value: Number(args.value),
        },
      ],
      Namespace: args.namespace || 'Coinboss',
    }).promise()
    .then(function(result) {
      console.log('putAverageMetricData', result);
    });
  }

This is where I get lost. I looks like the data never arrives at the putAverageMetricData function. The console output only shows me the console.log('tx', tx); with the following:

2017-07-15T19:37:43.670Z 118ff4f0-6995-11e7-8ae7-dd68094efbd6 tx [ undefined ]

Ok, so I was not returning the calculateMovingAverage() then. It solved the undefined error. I still don't get logging from the putAverageMetricData() function which leaves me to think I am still missing something.

I hope someone can point me into the right direction

jarmod
  • 71,565
  • 16
  • 115
  • 122
tvb
  • 783
  • 1
  • 10
  • 21
  • `ma.calculateMovingAverage` is not returning anything (so it returns `undefined`). Secondly, `Promise.all` is a but useless when you pass it an array with just one element. Just use `then` in that case. Also, shouldn't `resolve(ma.movingAverage());` be `resolve(self.ma.movingAverage());`? Did you check the console for errors? – trincot Jul 15 '17 at 20:28
  • The `Promise.all` is there, because as I understand I want both `ma.calculateMovingAverage()` to be handled. I only disabled one to keep it somewhat more simple. I think you are right. I added `self`. Unfortunately the console doesn't show me any logging that I added at that point. It only shows me the logging I am doing in the `then` of `export const calculateMovingAverage`. – tvb Jul 15 '17 at 20:41

3 Answers3

1

Your getMetricStatistics and putAverageMetricData methods already return promises, so avoid the Promise constructor antipattern in calculateMovingAverage! And don't forget to return a promise in the end from there:

calculateMovingAverage(exchange, metric) {
  const args = {
    minutes: 10,
    period: 60,
    currency: this.cryptoCurrency,
    metricname: metric,
    localCurrency: this.localCurrency,
    namespace: exchange,
  };
  return this.cloudwatch.getMetricStatistics(args).then(data => {
    if (!Array.isArray(data.Datapoints) || !data.Datapoints.length)
      throw new "No Datapoints received from CloudWatch!";

    for (const item of data.Datapoints) {
      this.ma.push(item.Timestamp, item.Average);
    }
    return this.ma.movingAverage();
  }).then(results => {
    const body = {
      value: results,
      metricName: `${metric} @ 180 MovingAverage`,
      namespace: exchange
    };
    return this.cloudwatch.putAverageMetricData(body);
  });
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thank you for this. It solved my issues. I do have some additional questions about the proposed code you've written. `const promisedland = this.cloudwatch.getMetricStatistics(args).then(data => {` You removed the error handling, why? This line: `throw new "No Datapoints received from CloudWatch!";` You changed the `reject` into a `throw new`, why? This line: `for (const item of data.Datapoints) {` You changed the forEach, why? – tvb Jul 16 '17 at 10:50
  • 1
    1) The `catch` was unnecessary since it didn't actually handle anything, also it called a `reject` function that was nowhere in scope so I guessed it's a leftover from the `Promise` constructor antipattern. 2) Again, there's no `reject` in scope - this time because I removed the promise constructor - and instead we just throw an exception from the `then` handler, which leads to a rejected promise as well. Notice I also changed the `resolve` call to a `return`. 3) `for of` is just simpler and faster (no function calls) – Bergi Jul 16 '17 at 11:51
  • yes, I noticed the change from `resolve` into a `return` as well. I was meant to ask you about that. Shouldn't I resolve `self.ma.movingAverage();` instead? I also changed `const promisedland = ` in a return and removed the `return promisedland.` Looks better. Again, thanks for the help and explanations! – tvb Jul 16 '17 at 14:56
  • 1
    @Tristan No, you should not call `resolve` (there's no function) but `return` from a `then` callback - the value will become the result of the promise returned by `.then(…)`. Regarding `self.ma`/`this.ma` vs `ma`, you had the second one in your original code so I kept that, it probably should be `this.ma` though (you don't need `self` as you can directly access the `this` in arrow functions). – Bergi Jul 16 '17 at 21:01
0

In calculateMovingAverage you have to return the promise you're creating, otherwise Promise.all can't tell when the promises are resolved.

return promisedland.then((results) => {
  ...
}).catch(...);
lleaff
  • 4,249
  • 17
  • 23
  • Thanks. I don't get the undefined error anymore. I still don't get the logging for `putAverageMetricData()` which leaves me to think it doesn't get called still. So I must be missing something else. – tvb Jul 15 '17 at 20:43
0

Instead of giving you a direct answer to how to fix this code I will give you the Promises 101 course and I think you will be able to understand what is going on at a higher level.

Callback Functions

JavaScript is (usually) "single threaded" meaning only one line of code can be executed at a time. Sometimes, we need to do things that take really long like making a request to our server. To handle this, javascript uses the callback function. A callback function is when you pass a function into another function as an argument. The most basic example is the settimout function.

setTimeout(function() {
  // settimout takes a callback function
}, 1000);

Now, the magic of the callback is it won't be executed until all the other "not callback" code or "synchronous" code

setTimeout(function(error, goodStuff) {
  console.log("WHAAT WHY AM I SECOND?") //this gets printed second
}, 1000);

console.log("YAY! I GET TO GO FIRST!!!!") // this is printed first

This is known as the javascript event loop. Here is a little picture I made to give you an idea of what it is:

The Javascript Event Loop

enter image description here

As you can see there is the call stack and the asynchronous queue. All of your code that is not a callback or asynchronous will go to the call stack. The synchronous trump functions are going to cut the line and be run first. The callback functions will go to the message queue or event loop and wait for all the synchronous functions to finish. Once the synchronous function's call stack completes, the callbacks will start executing. But eventually, javascript programmers ran into a little problem.

Callback Hell and Promises

Javascript started to get more and more complicated and eventually, the callbacks started taking callbacks. The more they became nested, the harder they became to read. Look at this eye sore:

fs.readdir(source, function (err, files) {
  if (err) {
    console.log('Error finding files: ' + err)
  } else {
    files.forEach(function (filename, fileIndex) {
      console.log(filename)
      gm(source + filename).size(function (err, values) {
        if (err) {
          console.log('Error identifying file size: ' + err)
        } else {
          console.log(filename + ' : ' + values)
          aspect = (values.width / values.height)
          widths.forEach(function (width, widthIndex) {
            height = Math.round(width / aspect)
            console.log('resizing ' + filename + 'to ' + height + 'x' + height)
            this.resize(width, height).write(dest + 'w' + width + '_' + filename, function(err) {
              if (err) console.log('Error writing file: ' + err)
            })
          }.bind(this))
        }
      })
    })
  }
})

So to make stuff like this easier to read the promise was born! It took code like the callback hell above and make it read more line by line.

myfirstPromise().then((resultFromFirstPromise) => {
    return mySecondPromise();
  }).then((resultFromSecondPromise) => {
    return myThirdPromise();
  }).then((resultFromThirdPromise) => {
    //do whatever with the result of the third promise
  }).catch((someError) => {
    //if any of the promises throw an error it will go here!
  })

So applying these concepts to your code this is what we want to do:

  getMetricStatistics(options)
    .then(metricStatistics => {
      // do what you need to do with those stats

    return putAverageMetricData(metricStatistics);
    })
    .then((putMetricDataResult => {
      //do what you need to do with the metric data result
    }))
    .catch(err => {
      //do error stuff
    })
EJ Mason
  • 2,000
  • 15
  • 15
  • 1
    the only question I have is do you need to run putAverageMetricData for each result? – EJ Mason Jul 16 '17 at 03:20
  • 1
    Thank you for your detailed answer. It helped me understand promises just a little bit more! – tvb Jul 16 '17 at 10:52