feat(#64) added multiple aggregate levels #66

mscottx88 · 2017-07-19T23:47:15Z

@jasonwilson Here is the first go at this code. I need to add unit tests for the new work. If you get a moment, check it out and see what you think!

jasonwilson · 2017-07-20T17:13:38Z

lib/dashboard.js

@@ -94,8 +100,8 @@ var VIEW_MAP = {
  eventLoop: EventLoopView
 };

-Dashboard.prototype._showLayout = function (id) {
-  if (this.currentLayout === id) {
+Dashboard.prototype._showLayout = function (id, forced) {


Is there another way to update the aggregate view without having to force a layout refresh?

I'm sure there is. I will work on it!

jasonwilson · 2017-07-20T20:03:33Z

.vscode/launch.json

@@ -0,0 +1,37 @@
+{
+  // Use IntelliSense to learn about possible Node.js debug attributes.


can we remove these comments?

i think this is a visual studio file - should add file/dir to .gitignore

jasonwilson · 2017-07-20T21:37:17Z

lib/providers/metrics-provider.js

+
+    screen.on("zoomAggregate", function zoomAggregate(zoom) {
+      // apply zoom delta and check for boundaries
+      this.currentAggregateZoom += zoom;


Could we just use something like lodash.clamp (https://lodash.com/docs/4.17.4#clamp) to enforce bounds?

aisapatino · 2017-07-20T18:24:45Z

lib/providers/metrics-provider.js

-var MetricsProvider = function MetricsProvider(screen) {
-  EventEmitter.call(this);
+// time scaling factors
+var timeScales = [


this could be in constants.js

aisapatino · 2017-07-20T21:42:35Z

.vscode/launch.json

@@ -0,0 +1,37 @@
+{
+  // Use IntelliSense to learn about possible Node.js debug attributes.


i think this is a visual studio file - should add file/dir to .gitignore

Also, corrected the use of sinon-chai and fixed a cross-platform issue with mock-require. Included gulp and gulpfile for unit test debugging. New constants.js file for configuration. Lots of performance improvements and code cleanup in MetricsProvider aggregation logic. Lots of documentation for MetricsProvider.

ryan-roemer · 2017-07-22T04:48:03Z

gulpfile.js

@@ -0,0 +1,9 @@
+"use strict";


Small nit here -- at least as a broad Formidable convention (and same for most of our clients), npm scripts have "won".

Gulp, while a great tool, has persistent problems of plugins being behind the "real" upstream projects and adding complexity / abstraction where it's not needed. Here, we just want to execute mocha. I vote we stick with mocha + package.json:scripts until we hit a use case we truly can't support.

Side note -- if we need concurrent execution of things, we've got great tools for that that work with package.json:scripts.

@jasonwilson -- Thoughts on ^^^ ?

Can npm run scripts allow you to debug your spec files? If so, that is great. Otherwise, the only purpose for the gulp is for debugging unit tests and I can keep it out of the main repo.

Although, excluding it would make portability an issue across dev machines.

First, you can always use mocha directly. Second npm scripts allow extra commands to punch to the command via --. So, for example,

$ npm run test-only -- --grep=foo --ANY_OTHER_MOCHA_CLI_FLAG

works just fine.

Also, we write our npm package scripts to be the portable subset of shell commands that is universally (mac, win, linux) compatible.

By dev machine, I really meant for those folks that might use VS code as their debugger, these tagalong files are helpful to include

I'm not familiar with VS code's debugging, but I'd be surprised if generic gulp was supported and raw mocha wasn't. In any case, let's keep the main repo gulp-free to save on dependencies we don't strictly need 😉

Sounds good. Also sounds like I might have another interested party in learning debugging and using VS code?

I think you'll find it to be a really amazing editor.

Since it is an Electron app, it is built on Node too, so this editor is perfectly adapted to Node development.

Out of curiosity, since you're not using VS code, what editor are you using?

I believe I have the solution I was looking for to remove the gulp and still get the debug ability.

https://gist.github.com/paambaati/54d33e409b4f7cf059cc

Up to date even!

I use many, currently vs code. What I meant is that I don't use the integrated debugger -- typically when I need a debugger I just do the normal node flags and open up chrome.

Nice to see you found what you need and your preferred debugging solution is still available to you!

ryan-roemer · 2017-07-22T04:51:09Z

test/lib/dashboard-agent.spec.js

-        expect(metrics.mem.heapTotal).to.be.above(0);
-        expect(metrics.mem.heapUsed).to.be.above(0);
-        expect(metrics.cpu.utilization).to.be.above(0);
+        expect(metrics.eventLoop.delay).to.be.a("number");


GLOBAL SUGGESTION: I know this is a refactor, but for things like:

expect(metrics.eventLoop.delay).to.be.a("number");

if for example, metrics.eventLoop is undefined, you're going to get a not-so-useful assertion failure. A better way to do this is to assert on the property and let chai tell you "that deep property doesn't exist". Aka

expect(metrics).to.have.deep.property("eventLoop.delay").that.is.a("number");

(Side note -- unique to .property assertions, the context changes from there on to the property asserted against (e.g. metrics.eventLoop.delay) and not the original target object (e.g. metrics))

Neat, I like it.

ryan-roemer · 2017-07-22T04:54:32Z

test/lib/dashboard-agent.spec.js

      };

+      var stubProcess = sinon.stub(process, "memoryUsage", function () {


GLOBAL: Never use sinon.stub|spy on anything that requires a .restore(). Instead, use a sandbox which we declare at top scope for this test suite.

Here you've got a problem that if anything fails / errors in this test, you won't reach the .restore()s on L99-L100. That means you have real methods potentially left unrestored that could cause unrelated tests to fail. Thus, you must always try/catch synchronous stubs to restore or do it in a beforeEach|afterEach pair.

Since we already do this with a sandbox, you can just use that to sandbox.stub and never worry about the restore, since a global restore is done in the top-level afterEach

I tried the sandbox but when the sandbox.restore() ran, the stub was still there. On other words, the sandbox did not work as expected.

We can work together on Monday. A sandbox restores anything created with it.

Also, @jasonwilson and @aisapatino can help with this as they've both got a ton of experience with mocha + sandboxes.

Aha, I know what I did wrong, instead of sandbox.stub() I used sinon.stub() and that would explain why it didn't go in the sandbox.

I'll correct this one, thanks!

One thing I did notice was the sandbox create was in a before() (pre-existing code). This may be problematic, and as a general rule for unit tests, beforeEach() is almost always better than a before(). before() is typically reserved for super-expensive time-wise things, and I don't see that here. (Same for after())

ryan-roemer · 2017-07-22T04:55:10Z

test/lib/generate-layouts.spec.js

@@ -149,6 +154,8 @@ describe("generate-layouts", function () {
        ]
      }
    ]]);
+
+    done();


This looks unnecessary.

ryan-roemer · 2017-07-22T04:55:23Z

test/lib/providers/metrics-provider.spec.js

+      });
+    }
+
+    done();


GLOBAL: done() should only be used for async callbacks. Everywhere that it's been added seems synchronous.

Sounds good. I've always done the done() as a consistency thing, but easily removed!

ryan-roemer · 2017-07-22T04:55:29Z

test/lib/providers/metrics-provider.spec.js

  });

-  beforeEach(function () {
+  after(function (done) {
+    done();


What is this?

ryan-roemer · 2017-07-22T04:57:49Z

test/lib/views/eventloop-view.spec.js

 var sinon = require("sinon");
+var sinonChai = require("sinon-chai");
+var expect = chai.expect;
+chai.use(sinonChai);


GLOBAL: This shouldn't be necessary -- this boilerplate is already taken care of in setup.js https://github.com/FormidableLabs/nodejs-dashboard/blob/master/test/setup.js

I think maybe the gulp switch isn't using setup? At any rate go with how we already ran mocha from package.json scripts and should work fine.

I tried npm run scripts in addition to gulp and the problem was still there.

We can work through it on Monday. If we've got you Slack access, just hit me or @jasonwilson up and we'll work it out.

And generally, speaking, anytime you're doing a repeated, fundamental change to a bunch of files, feel free to bounce that off of us early so we can possibly help debug things up front and save you some time!

Hmm I wonder, and this could be a red herring, but is it possibly related to the order of the parameters on the npm script? I see a difference between the coverage script and test-only script.

Scratch that, I see the commands aren't the same.

See gotwarlost/istanbul#90 for reasoning

mscottx88 · 2017-07-24T19:38:08Z

@ryan-roemer @jasonwilson @aisapatino I believe this is at last ready for final review :)

ryan-roemer · 2017-07-24T20:25:57Z

@mscottx88 -- Great! I'll review for formalities / test stuff and @jasonwilson @aisapatino can do the substantive stuff in addition to that.

ryan-roemer

Getting there!

ryan-roemer · 2017-07-24T20:24:42Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Change log

+## [v0.5.1] - 2017-07-24


NIT: Typically we do Unreleased until we release. Here, it would seem our options should be MINOR (v0.4.2) or MAJOR (v0.5.0) in that v0.x.x semver parlance or whatever 😛

yea i meant 5.0.0... sorry about that. Should I put Unreleased for now or leave CHANGLOG unchanged as part of this PR?

Put Unreleased here. Leave package.json:version unchanged from current master.

Cool, I will correct this one!

ryan-roemer · 2017-07-24T20:25:08Z

lib/constants.js

+
+// these define the time levels that data will be aggregated into
+// each number is in milliseconds
+


NIT: Do the empty newline // too, so that it reads as a contiguous "comment"

ryan-roemer · 2017-07-24T21:10:26Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "nodejs-dashboard",
-  "version": "0.4.1",
+  "version": "0.5.0",


Again, typically we don't bump versions in PRs. Also, this differs from changelog entry.

ryan-roemer · 2017-07-24T21:10:55Z

package.json

@@ -16,7 +16,7 @@
    "test": "npm run lint && npm run test-only",
    "test-only": "mocha -c --require test/setup.js --recursive ./test",
    "test-app": "./bin/nodejs-dashboard.js node test/app/index.js",


Not part of this PR, but can you remove the prefix ./ in this command? ./ is unnecessary on mac/linux and breaks windows.

This one is weird. As is, with the ./, I get the following error:

./bin/nodejs-dashboard.js node test/app/index.js

'.' is not recognized as an internal or external command,
operable program or batch file.

When I change it to not have the ./ I get this error instead:

[email protected] test-app C:\Users\formidable\Documents\nodejs-dashboard
bin/nodejs-dashboard.js node test/app/index.js

'bin' is not recognized as an internal or external command,
operable program or batch file.

Maybe my npm settings are incorrect. When I put node in front, it works:

$ npm run test-app

[email protected] test-app C:\Users\formidable\Documents\nodejs-dashboard
node bin/nodejs-dashboard.js node test/app/index.js

Waiting for client connection on 9838...

@ryan-roemer Should/Could it be done as:

"test-app": "node bin/nodejs-dashboard.js -- node test/app/index.js",

@jasonwilson -- Can you comment here? The problem is that bin/nodejs-dashboard.js isn't a recognizable Windows cmd script.

I'm guessing we want:

node bin/nodejs-dashboard.js node test/app/index.js

but not totally sure...

Cool, that's what I ended up with too!

ryan-roemer · 2017-07-24T21:13:02Z

test/lib/generate-layouts.spec.js

 var mock = function (path, obj) {
-  return mockRequire(process.cwd() + "/" + path, obj);
+  return mockRequire(normalize(process.cwd() + "/" + path), obj);


The existing string concat with "/" feels wrong.

Wouldn't

return mockRequire(path.resolve(path), obj);

solve all the problems we have here?

mscottx88 · 2017-07-24T22:25:10Z

@ryan-roemer I reverted the version bump and changed from normalize to resolve.

mscottx88 · 2017-07-25T15:54:10Z

Woohoo! I figured out how to get the screen to update without the forced option in showLayouts. All I needed was a lil bit of inversion of control. Turns out the BaseLineGraph needs to request the data, so when changing the zoom level, the higher classes provide a mapper function.

jasonwilson · 2017-07-25T23:41:21Z

The units seem to be offset from whole intervals. Can we make these fixed?

mscottx88 · 2017-07-25T23:47:11Z

Do you mean, keep them as seconds until everything is evenly divisible by minutes, and so on?

mscottx88 · 2017-07-26T01:17:01Z

It turns out that 7.3m = 438s, both of which take up the same space on the screen 🤣

jasonwilson · 2017-07-26T03:09:14Z

I mean so the units are fixed, whole number increments. For example: 1m 5m 10m 15m 20m 25m

Time labels now appear like digital clocks.

mscottx88 · 2017-07-26T19:39:44Z

mscottx88 · 2017-07-26T22:06:37Z

I've got the new #67 ready to go, in a separate branch, but it relies on this one.

The memory guage view was not getting its "metrics" events and appears frozen. Since it is not a historical graph, the "metrics" event needed to be emitted every time.

mscottx88 · 2017-08-07T22:42:55Z

@jasonwilson @aisapatino Hi there, I was just following up on this PR. I have a few more branches/features I'd like to merge after this one.

jasonwilson

A couple small changes then we're good to go!

jasonwilson · 2017-08-08T04:17:49Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Change log

+## [Unreleased] - 2017-07-24
+- **Added:** Longer history for graphs [\#64]


Need to include the PR reference at the bottom of the file.

jasonwilson · 2017-08-08T04:19:02Z

lib/providers/metrics-provider.js

@@ -1,25 +1,500 @@
 "use strict";

+/**
+ * This module provides metric data for various data points.


Lets put this writeup in a separate markdown file and abbreviate the explanation here.

jasonwilson · 2017-08-08T04:29:14Z

lib/views/base-line-graph.js

@@ -71,6 +77,38 @@ BaseLineGraph.prototype.update = function (values) {
  this.node.setData(_.values(this.series));
 };

+BaseLineGraph.prototype.changeMetricData = function (mapper) {


Anyway to have blessed fix the increments?

jasonwilson · 2017-08-08T04:31:04Z

test/lib/dashboard-agent.spec.js

      };

+      sandbox.stub(process, "memoryUsage", function () {
+        return {
+          systemTotal: 20,


mscottx88 · 2017-08-08T17:13:28Z

@jasonwilson Corrected those, thanks for the recommendation on the separate MD. I was able to include some visuals which I really wanted to do anyway 👍

ryan-roemer

Nice work!

jasonwilson · 2017-08-08T17:52:24Z

🚀

feat(FormidableLabs#64) added multiple aggregate levels

f5666af

jasonwilson reviewed Jul 20, 2017

View reviewed changes

aisapatino reviewed Jul 20, 2017

View reviewed changes

mscottx88 and others added 3 commits July 20, 2017 14:45

fix(FormidableLabs#64) remove vscode file first, then gitignore it

22bc27a

fix(FormidableLabs#64) gitignore vs code

bdd3adf

ryan-roemer reviewed Jul 22, 2017

View reviewed changes

test/lib/providers/metrics-provider.spec.js Outdated

});

beforeEach(function () {

after(function (done) {

done();

Copy link

Member

ryan-roemer Jul 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this?

ryan-roemer reviewed Jul 22, 2017

View reviewed changes

mscottx88 added 7 commits July 22, 2017 12:08

test(FormidableLabs#64) removed gulp, removed duplicate setup

64e7805

test(FormidableLabs#64) removed unnecessary done()

89a6fa2

fix(FormidableLabs#64) test use sandbox stub

07871d1

fix(FormidableLabs#64) cross-platform fix to coverage

f3b8f2c

See gotwarlost/istanbul#90 for reasoning

chore(FormidableLabs#64) simplified aggregation

6dce469

chore(FormidableLabs#64) converted to inline aggregation

7b373a1

test(FormidableLabs#64) 100% code coverage MetricsProvider

6dd1f3b

docs(FormidableLabs#64) bumped version and added changelog entry

939ba75

ryan-roemer suggested changes Jul 24, 2017

View reviewed changes

fix(FormidableLabs#64) corrections for code review

403853a

mscottx88 added 2 commits July 24, 2017 15:43

docs(FormidableLabs#64) added notes to Changelog

b7eec6c

fix(FormidableLabs#64) no more forced layouts

ae31391

fix(FormidableLabs#64) improved time labels

1b61876

Time labels now appear like digital clocks.

mscottx88 added 2 commits July 26, 2017 13:21

fix(FormidableLabs#64) getTimeIndexLabel is not a member function

d749914

fix(FormidableLabs#64) lodash has padStart

07ddccb

fix(FormidableLabs#64) memory guage view frozen

dbd091e

The memory guage view was not getting its "metrics" events and appears frozen. Since it is not a historical graph, the "metrics" event needed to be emitted every time.

jasonwilson suggested changes Aug 8, 2017

View reviewed changes

Michael P. Scott added 2 commits August 8, 2017 10:10

docs(FormidableLabs#64) separate MD for aggregation

b3f7319

docs(FormidableLabs#64) correct PR ref

20c542f

jasonwilson approved these changes Aug 8, 2017

View reviewed changes

ryan-roemer approved these changes Aug 8, 2017

View reviewed changes

jasonwilson merged commit 4ad9351 into FormidableLabs:master Aug 8, 2017

mscottx88 deleted the features/graph-units-of-time branch August 8, 2017 18:16

		@@ -0,0 +1,37 @@
		{
		// Use IntelliSense to learn about possible Node.js debug attributes.

		};

		var stubProcess = sinon.stub(process, "memoryUsage", function () {

@@ @@ -149,6 +154,8 @@ describe("generate-layouts", function () { @@
                       ]
                     }
                   ]]);
+                  done();

+                    });
+                  }
+                  done();


		// these define the time levels that data will be aggregated into
		// each number is in milliseconds

feat(#64) added multiple aggregate levels #66

feat(#64) added multiple aggregate levels #66

Conversation

mscottx88 commented Jul 19, 2017

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

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

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

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

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

mscottx88 commented Jul 24, 2017

ryan-roemer commented Jul 24, 2017

ryan-roemer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryan-roemer Jul 24, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscottx88 commented Jul 24, 2017

mscottx88 commented Jul 25, 2017

jasonwilson commented Jul 25, 2017

mscottx88 commented Jul 25, 2017

mscottx88 commented Jul 26, 2017

jasonwilson commented Jul 26, 2017

mscottx88 commented Jul 26, 2017

mscottx88 commented Jul 26, 2017

mscottx88 commented Aug 7, 2017

jasonwilson left a 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

mscottx88 commented Aug 8, 2017

ryan-roemer left a comment

Choose a reason for hiding this comment

jasonwilson commented Aug 8, 2017

ryan-roemer Jul 24, 2017 •

edited

Loading