Fix average functions duration calculation in metrics output #3067

vladholubiev · 2017-01-06T21:21:07Z

What did you implement:

Closes #3066 + refactoring of AWS Metrics plugin

How did you implement it:

Before:
Previously it was summing durations from each day and dividing it by number of Lambda functions, instead of number of data points from CloudWatch.

After:

const durationAverage = _.meanBy(getDatapointsByLabel('Duration'), 'Average') || 0;

Also refactored some code to increase readability (I hope) and "code churn" metric 😅 I was careful and all the tests are still passing, so should be ok.

How can we verify it:

This can be verified by:

Unit test: should display correct average of service wide average function duration
By manually checking service wide metrics for a large period of time for 1 function. Previously you would see abnormally high value.

Also I'm not sure whether the code coverage dropped or not. It dropped by statements, but increased by branches.

before:
Statements   : 95.63% ( 2624/2744 )
Branches     : 90.26% ( 1056/1170 )
Functions    : 97.55% ( 279/286 )
Lines        : 95.71% ( 2587/2703 )


after:
Statements   : 95.53% ( 2587/2708 )
Branches     : 90.72% ( 1046/1153 )
Functions    : 97.56% ( 280/287 )
Lines        : 95.62% ( 2552/2669 )

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification config/commands/resources
Enable "Allow edits from maintainers" for this PR
Change ready for review message below

Is this ready for review?: YES
Is it a breaking change?: NO

…tion duration

vladholubiev · 2017-01-06T21:42:59Z

Also docs looks strange (2001 and 2002). But they are ok in repo.

pmuens · 2017-01-07T12:44:09Z

Thank you very much @vladgolubev you rock 🎸 !

Awesome PR description! 👍 We'll look into it ASAP!

pmuens

Really, really good refactoring!

Nicely done @vladgolubev love it (I always love it when more code is removed rather than added) 🥇 👍

I added 2 minor comments. Usually it's better to have function based PRs (so in this case one PR for the refactoring of the metrics plugin, one for awsProvider, one for configCredentials etc. which makes the PR easier to review and also easier to track down specific PRs when they introduce bugs) but these changes are not that major so it's fine to merge them here as well.

Again great job @vladgolubev GTM from my side once the comments are discussed / resolved 👍

pmuens · 2017-02-01T12:53:36Z

lib/classes/Service.js

@@ -182,6 +182,10 @@ class Service {
    return Object.keys(this.functions);
  }

+  getAllFunctionsNames() {


This should be covered by a test.

Of course, thanks @pmuens, just pushed the changes

pmuens · 2017-02-01T12:58:17Z

lib/plugins/aws/configCredentials/awsConfigCredentials.js

-      (env.HOMEPATH ? ((env.HOMEDRIVE || 'C:/') + env.HOMEPATH) : null);
-
-    if (!home) {
+    if (!os.homedir()) {


is os.homedir() also safe for old Node versions?

Here's the list of Node versions we "officially" support:

serverless/.travis.yml

Lines 5 to 8 in 53c9608

- node_js: '4.4'

- node_js: '5.11'

- node_js: '6.2'

- node_js: '6.2'

Hm, added in v2.3.0 - means safe.

My reasoning was, that I noticed that unit tests are using os.homedir() but the code not. After git blame found out it was because after refactoring project structure file was just copy pasted, and tests were written after by a different person. So just minor inconsistency.

Yep. That totally makes sense.

I copied the code for the homedir detection from an old PR and pushed the test on top of that 😬. But yes, if the test use os.homedir and work for all node versions on our CI system, then the usage of os.homedir should work in the main code as well 😆 🤦‍♂️

vladholubiev · 2017-02-07T14:57:42Z

@pmuens thanks for the review and feedback! Sorry for absence, I'm back here )

Usually it's better to have function based PRs

I strongly agree for that!! But with this PR the story was a bit different.

When you released metrics support in some of the recent releases, I, was impressed. So I went to the sources to read the implementation. After reading, I created a note to myself to refactor that file, as it's a great functionality, but no so readable as I could wish.

So I started refactoring it at that time, and after unit tests, I moved to testing it with my AWS account.

And at that time I realised that outputs differ from my local sls branch and what is published to npm. So I started digging into it to find where is my bug.

But in the end, it occurred on contrary - my refactoring fixed a bug I didn't have an idea about.

So, refactoring without any purpose resulted in a bug fix. Of course, afterwards I could fix couple of lines in the old code knowing the reason, and submit a new PR with just a refactoring, buuut, this didn't happen, it wasn't already so interesting for me, so all I can say to forgive me

pmuens

Thanks for the update @vladgolubev 👍

Just gave it another spin and it works as expected. 💯

so all I can say to forgive me

No hard feelings! We love the stuff you do you so keep up the great work 🥇 👍

pmuens · 2017-02-08T13:02:15Z

lib/classes/Service.js

@@ -182,6 +182,10 @@ class Service {
    return Object.keys(this.functions);
  }

+  getAllFunctionsNames() {


pmuens · 2017-02-08T13:04:03Z

lib/plugins/aws/configCredentials/awsConfigCredentials.js

-      (env.HOMEPATH ? ((env.HOMEDRIVE || 'C:/') + env.HOMEPATH) : null);
-
-    if (!home) {
+    if (!os.homedir()) {


Yep. That totally makes sense.

I copied the code for the homedir detection from an old PR and pushed the test on top of that 😬. But yes, if the test use os.homedir and work for all node versions on our CI system, then the usage of os.homedir should work in the main code as well 😆 🤦‍♂️

eahefnawy

@vladgolubev I love how extensive your lodash usage is! ❤️ It helps refactor tons of LOCs

Approved!

eahefnawy · 2017-02-14T13:06:46Z

lib/plugins/aws/provider/awsProvider.js

+    return _.get(this, 'options.region')
+      || _.get(this, 'serverless.config.region')
+      || _.get(this, 'serverless.service.provider.region')
+      || defaultRegion;


eahefnawy · 2017-02-14T13:07:29Z

lib/plugins/aws/metrics/awsMetrics.js

+      const invocationsCount = _.sumBy(getDatapointsByLabel('Invocations'), 'Sum');
+      const throttlesCount = _.sumBy(getDatapointsByLabel('Throttles'), 'Sum');
+      const errorsCount = _.sumBy(getDatapointsByLabel('Errors'), 'Sum');
+      const durationAverage = _.meanBy(getDatapointsByLabel('Duration'), 'Average') || 0;


vladholubiev added 19 commits January 6, 2017 18:24

Use moment.js to set default yesterday date

f3c09ef

Use regex to check if date is in since format

b880acd

Use moment.js to format date in metrics tests

59051c1

Simplify validating function name in metrics

f1fdbcf

Add getAllFunctionsNames to Service class

37f4545

Further refactor parsing human start dates in metrics

1c3dbd4

Refactor calculating metrics counts

c86e09a

Refactor getting aws provider region

0fb4cf6

Refactor getting aws provider stage

7d7f7c7

Add test: should display correct average of service wide average func…

f2eaae5

…tion duration

Round average function duration to 2 digits after dot

d72b951

Remove unused stage and region arguments from provider request

f090f86

Display 0 as average function duration if no data by given period

f2ff206

Refactor getMetrics method to reduce duplication

39b890d

Fix ESLint errors

9385f96

Fix getMetrics test by wrapping date, as we don't call validate

c2a6b4a

Add tests to verify valid metrics Period param

184d4fe

Use os.homedir() instead of own implementation

e1d8d7a

Fix start / end date in metrics tests

f0ee493

pmuens added this to the 1.7 milestone Jan 18, 2017

pmuens self-requested a review January 18, 2017 12:39

pmuens reviewed Feb 1, 2017

View reviewed changes

vladholubiev added 3 commits February 7, 2017 23:41

Merge branch 'master' into fix-average-functions-duration

e8bf959

Merge branch 'master' into fix-average-functions-duration

12ba454

Add missing unit tests for #getAllFunctionsNames

9a8a9dd

pmuens approved these changes Feb 8, 2017

View reviewed changes

pmuens added the stage/in-review label Feb 13, 2017

eahefnawy approved these changes Feb 14, 2017

View reviewed changes

eahefnawy merged commit cc94945 into serverless:master Feb 14, 2017

vladholubiev deleted the fix-average-functions-duration branch February 14, 2017 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix average functions duration calculation in metrics output #3067

Fix average functions duration calculation in metrics output #3067

vladholubiev commented Jan 6, 2017

vladholubiev commented Jan 6, 2017 •

edited

pmuens commented Jan 7, 2017 •

edited

pmuens left a comment •

edited

pmuens Feb 1, 2017

vladholubiev Feb 7, 2017

pmuens Feb 8, 2017

pmuens Feb 1, 2017

pmuens Feb 1, 2017

vladholubiev Feb 7, 2017

pmuens Feb 8, 2017

vladholubiev commented Feb 7, 2017 •

edited by pmuens

pmuens left a comment •

edited

pmuens Feb 8, 2017

pmuens Feb 8, 2017

eahefnawy left a comment

eahefnawy Feb 14, 2017

eahefnawy Feb 14, 2017

	- node_js: '4.4'
	- node_js: '5.11'
	- node_js: '6.2'
	- node_js: '6.2'

Fix average functions duration calculation in metrics output #3067

Fix average functions duration calculation in metrics output #3067

Conversation

vladholubiev commented Jan 6, 2017

What did you implement:

How did you implement it:

How can we verify it:

Todos:

vladholubiev commented Jan 6, 2017 • edited

pmuens commented Jan 7, 2017 • edited

pmuens left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladholubiev commented Feb 7, 2017 • edited by pmuens

pmuens left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eahefnawy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladholubiev commented Jan 6, 2017 •

edited

pmuens commented Jan 7, 2017 •

edited

pmuens left a comment •

edited

vladholubiev commented Feb 7, 2017 •

edited by pmuens

pmuens left a comment •

edited