Record and report success metrics #15084

iarna · 2016-12-01T03:27:42Z

coveralls · 2016-12-07T04:33:45Z

Coverage increased (+0.08%) to 85.723% when pulling 3e6fe06 on iarna/anon-cli-stats into 4d0473c on release-next.

legodude17 · 2016-12-07T17:06:50Z

Nice idea! Is this implemented on the registry yet?

othiym23

Here's what I've got thus far – nothing in here should be construed as a blocker to merging this, but I am curious about your responses.

othiym23 · 2016-12-07T18:55:57Z

lib/utils/error-handler.js

  if (!npm.config || !npm.config.loaded) return
+
+  // kill any outstanding stats reporter if it hasn't finished yet
+  if (npm.statsProcess) npm.statsProcess.kill('SIGKILL')


I get the intent of this, but given that the main of lib/utils/send-metrics.js has to require most of npm to find the stats, what happens when somebody runs a quick command like npm -v?

Should this instead be using a trappable signal, like SIGINT or SIGTERM, and at least giving send-metrics a chance to clean up after itself?

send-metrics is explicitly written with the intent that killing it should be safe. I consider it more important that we be certain that there aren't any lingering processes (that are maybe updating the stats file) then that send-metrics not be interrupted.

othiym23 · 2016-12-07T18:58:29Z

lib/utils/send-metrics.js

+function main () {
+  var fs = require('fs')
+  var path = require('path')
+  var npm = require('../npm.js')


This makes metrics delivery a much more heavyweight process than it would be otherwise. How difficult would it be to just load the bits of config we need to bootstrap a registry client and create it directly? If the detached client were lighter-weight, I'd be less concerned about killing it with fire in the parent process's exit handler.

othiym23 · 2016-12-07T19:04:55Z

lib/utils/send-metrics.js

+  npm.load({}, function (err) {
+    if (err) return
+    npm.registry.config.retry.retries = 0
+    npm.registry.sendAnonymousCLIMetrics(metricsRegistry, cliMetrics, function (err) {


The situation I'm specifically concerned about is the metrics reporter getting killed between this line and clearing out the stats file below, which seems like a distinct possibility for faster-running comands (and is something that's likely to be exacerbated rather than mitigated by performance improvements to the rest of the CLI). Reporting the same stats multiple times isn't the worst thing in the world, but it seems like something we should be able to guard against more robustly.

This is intended to be allowed, per the spec I wrote, without double counting. This is why the metrics file gets a unique ID that's changed only when sending the CLI metrics is successful.

Without a succesfull send we update the metrics file without changing that unique ID. This way the registry can just count the most recent set of data with that unique ID.

Excellent! Consider my comment withdrawn, then.

othiym23 · 2016-12-07T20:53:12Z

test/tap/anon-cli-metrics.js

+})
+
+test('record success', function (t) {
+  common.npm(['install', '--no-send-metrics', 'success'], conf, function (err, code, stdout, stderr) {


I like that the tests don't rely on specific knowledge of what the metrics file looks like. This is a good set of integration tests! But running three installs to check one piece of behavior feels like a lot of overhead for the results. I don't think whacking this (and the next) install to speed up the test is a blocker, but if it could be done without a huge amount of trouble, it would be beneficial.

One set of behavior? I'm not sure I follow? There's no way to tell the installer to do all (or any) of these things all in one go…

I hadn't considered the fact that you're testing that the stats are being written correctly. With that in mind, my objection makes much less sense. Never mind!

othiym23 · 2016-12-07T20:56:45Z

lib/install.js

+  var statsFile = path.join(npm.config.get('cache'), 'anonymous-cli-stats.json')
+  var stats
+  try {
+    stats = JSON.parse(fs.readFileSync(statsFile))


Is it premature to abstract the metrics out into a defined interface, if our goal is to introduce more supportability metrics over time? In other words, would it make sense to put the metrics object into a module that can be required as needed across the project (or maybe hung off a process.on listener, a la numbat)?

I'm fine with shipping this as-is, but it does seem like this is something we're going to want to change pretty soon, and having it behind a defined interface from the get-go therefore makes a certain amount of sense.

I've just pushed a little refactor that moves all of the existing logic into a single file (without changing any of those interfaces), which makes this a bit clearer.

At the moment the entire interface that npm has is saveMetrics with true or false. And yeah, that's clearly not going to be sufficient going forward.

On the other hand, I think I'm inclined to leave it as is until we add another metric. My inclination is to wait until we have a concrete addition that necessitates the change in interface. Right now it'd be pretty hypothetical. We can make educated guesses, sure, but front loading the work uh... hmm, doesn't right?

I think that the latest refactor is more than sufficient for now. Thanks for the clear answers!

coveralls · 2016-12-09T01:52:01Z

Coverage increased (+0.08%) to 85.723% when pulling 4dfb41ee54c016ea205e4b7010041784d9fd9fd1 on iarna/anon-cli-stats into 87afc8b on release-next.

othiym23 · 2016-12-09T02:02:00Z

After this round of review, this is ready to merge. 🐑🚀🔥

coveralls · 2016-12-09T02:19:34Z

Coverage increased (+0.1%) to 85.743% when pulling 5c893ac30ce3cfc85eb1f1a10a007325daec6501 on iarna/anon-cli-stats into 87afc8b on release-next.

@iarna

Fixes: #12529 Credit: @iarna Reviewed-By: @othiym23

iarna · 2016-12-09T02:45:20Z

@legodude17 Nope. You can see the issue for tracking that part over here → npm/registry#11

coveralls · 2016-12-09T03:03:21Z

Coverage increased (+0.1%) to 85.743% when pulling 38e5c4f on iarna/anon-cli-stats into 87afc8b on release-next.

iarna added needs-tests review target-latest labels Dec 1, 2016

zkat force-pushed the release-next branch 4 times, most recently from e1c7ae8 to 04d7c2f Compare December 2, 2016 01:05

iarna force-pushed the iarna/anon-cli-stats branch 4 times, most recently from df3ff9e to 3e6fe06 Compare December 7, 2016 01:54

iarna removed the needs-tests label Dec 7, 2016

othiym23 reviewed Dec 7, 2016

View reviewed changes

[email protected]

eaeb7a8

iarna force-pushed the iarna/anon-cli-stats branch from 3e6fe06 to 4dfb41e Compare December 9, 2016 01:23

metrics: Record and report success metrics

38e5c4f

Fixes: #12529 Credit: @iarna Reviewed-By: @othiym23

iarna force-pushed the iarna/anon-cli-stats branch from 5c893ac to 38e5c4f Compare December 9, 2016 02:46

iarna merged commit 38e5c4f into release-next Dec 9, 2016

zkat mentioned this pull request Jan 26, 2017

deps: upgrade npm to 4.1.2 nodejs/node#11020

Closed

4 tasks

iarna deleted the iarna/anon-cli-stats branch March 7, 2017 23:10

Record and report success metrics #15084

Record and report success metrics #15084

Uh oh!

Conversation

iarna commented Dec 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Dec 7, 2016

Uh oh!

legodude17 commented Dec 7, 2016

Uh oh!

othiym23 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 9, 2016

Uh oh!

othiym23 commented Dec 9, 2016

Uh oh!

coveralls commented Dec 9, 2016

Uh oh!

iarna commented Dec 9, 2016

Uh oh!

coveralls commented Dec 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iarna commented Dec 1, 2016 •

edited

Loading