Skip to content

Commit

Permalink
metrics: Record and report success metrics
Browse files Browse the repository at this point in the history
Fixes: #12529
Credit: @iarna
Reviewed-By: @othiym23
  • Loading branch information
iarna committed Dec 13, 2016
1 parent eaeb7a8 commit 190a658
Show file tree
Hide file tree
Showing 8 changed files with 290 additions and 1 deletion.
17 changes: 17 additions & 0 deletions doc/misc/npm-config.md
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,13 @@ Commit message which is used by `npm version` when creating version commit.

Any "%s" in the message will be replaced with the version number.

### metrics-registry

* Default: "https://registry.npmjs.org/"
* Type: String

The registry you want to send cli metrics to if `send-metrics` is true.

### node-version

* Default: process.version
Expand Down Expand Up @@ -844,6 +851,16 @@ Space-separated options that are always passed to search.

The age of the cache, in seconds, before another registry request is made.

### send-metrics

* Default: false
* Type: Boolean

If true, success/failure metrics will be reported to the registry stored in
`metrics-registry`. These requests contain the number of successful and
failing runs of the npm CLI and the time period overwhich those counts were
gathered. No identifying information is included in these requests.

### shell

* Default: SHELL environment variable, or "bash" on Posix, or "cmd" on
Expand Down
4 changes: 4 additions & 0 deletions lib/config/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ Object.defineProperty(exports, 'defaults', {get: function () {
long: false,
maxsockets: 50,
message: '%s',
'metrics-registry': 'https://registry.npmjs.org/',
'node-version': process.version,
'onload-script': false,
only: null,
Expand Down Expand Up @@ -196,6 +197,7 @@ Object.defineProperty(exports, 'defaults', {get: function () {
searchopts: '',
searchexclude: null,
searchstaleness: 15 * 60,
'send-metrics': false,
shell: osenv.shell(),
shrinkwrap: true,
'sign-git-tag': false,
Expand Down Expand Up @@ -279,6 +281,7 @@ exports.types = {
long: Boolean,
maxsockets: Number,
message: String,
'metrics-registry': String,
'node-version': [null, semver],
'onload-script': [null, String],
only: [null, 'dev', 'development', 'prod', 'production'],
Expand All @@ -303,6 +306,7 @@ exports.types = {
searchopts: String,
searchexclude: [null, String],
searchstaleness: Number,
'send-metrics': Boolean,
shell: String,
shrinkwrap: Boolean,
'sign-git-tag': Boolean,
Expand Down
8 changes: 7 additions & 1 deletion lib/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ var unlock = locker.unlock
var ls = require('./ls.js')
var parseJSON = require('./utils/parse-json.js')
var output = require('./utils/output.js')
var saveMetrics = require('./utils/metrics.js').save

// install specific libraries
var copyTree = require('./install/copy-tree.js')
Expand Down Expand Up @@ -216,9 +217,14 @@ function Installer (where, dryrun, args) {
}
Installer.prototype = {}

Installer.prototype.run = function (cb) {
Installer.prototype.run = function (_cb) {
validate('F', arguments)

var cb = function (err) {
saveMetrics(!err)
return _cb.apply(this, arguments)
}

// FIXME: This is bad and I should feel bad.
// lib/install needs to have some way of sharing _limited_
// state with the things it calls. Passing the object is too
Expand Down
3 changes: 3 additions & 0 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
var cmdList = require('./config/cmd-list').cmdList
var plumbing = require('./config/cmd-list').plumbing
var output = require('./utils/output.js')
var startMetrics = require('./utils/metrics.js').start

npm.config = {
loaded: false,
Expand Down Expand Up @@ -308,6 +309,8 @@
// go ahead and spin up the registry client.
npm.registry = new CachingRegClient(npm.config)

startMetrics()

return cb(null, npm)
})
})
Expand Down
5 changes: 5 additions & 0 deletions lib/utils/error-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,15 @@ var rollbacks = npm.rollbacks
var chain = require('slide').chain
var writeStreamAtomic = require('fs-write-stream-atomic')
var errorMessage = require('./error-message.js')
var stopMetrics = require('./metrics.js').stop

process.on('exit', function (code) {
log.disableProgress()
if (!npm.config || !npm.config.loaded) return

// kill any outstanding stats reporter if it hasn't finished yet
stopMetrics()

if (code) itWorked = false
if (itWorked) log.info('ok')
else {
Expand Down
39 changes: 39 additions & 0 deletions lib/utils/metrics-launch.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict'
module.exports = launchSendMetrics
var fs = require('graceful-fs')
var child_process = require('child_process')

if (require.main === module) main()

function launchSendMetrics () {
var path = require('path')
var npm = require('../npm.js')
try {
if (!npm.config.get('send-metrics')) return
var cliMetrics = path.join(npm.config.get('cache'), 'anonymous-cli-metrics.json')
var targetRegistry = npm.config.get('metrics-registry')
fs.statSync(cliMetrics)
return runInBackground(__filename, [cliMetrics, targetRegistry])
} catch (ex) {
// if the metrics file doesn't exist, don't run
}
}

function runInBackground (js, args, opts) {
if (!args) args = []
args.unshift(js)
if (!opts) opts = {}
opts.stdio = 'ignore'
opts.detached = true
var child = child_process.spawn(process.execPath, args, opts)
child.unref()
return child
}

function main () {
var sendMetrics = require('./metrics.js').send
var metricsFile = process.argv[2]
var metricsRegistry = process.argv[3]

sendMetrics(metricsFile, metricsRegistry)
}
67 changes: 67 additions & 0 deletions lib/utils/metrics.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
'use strict'
exports.start = startMetrics
exports.stop = stopMetrics
exports.save = saveMetrics
exports.send = sendMetrics

var fs = require('fs')
var path = require('path')
var npm = require('../npm.js')
var uuid = require('uuid')

function startMetrics () {
// loaded on demand to avoid any recursive deps when `./metrics-launch` requires us.
var metricsLaunch = require('./metrics-launch.js')
npm.metricsProcess = metricsLaunch()
}

function stopMetrics () {
if (npm.metricsProcess) npm.metricsProcess.kill('SIGKILL')
}

function saveMetrics (itWorked) {
// If the metrics reporter hasn't managed to PUT yet then kill it so that it doesn't
// step on our updating the anonymous-cli-metrics json
stopMetrics()
var metricsFile = path.join(npm.config.get('cache'), 'anonymous-cli-metrics.json')
var metrics
try {
metrics = JSON.parse(fs.readFileSync(metricsFile))
metrics.metrics.to = new Date().toISOString()
if (itWorked) {
++metrics.metrics.successfulInstalls
} else {
++metrics.metrics.failedInstalls
}
} catch (ex) {
metrics = {
metricId: uuid.v4(),
metrics: {
from: new Date().toISOString(),
to: new Date().toISOString(),
successfulInstalls: itWorked ? 1 : 0,
failedInstalls: itWorked ? 0 : 1
}
}
}
try {
fs.writeFileSync(metricsFile, JSON.stringify(metrics))
} catch (ex) {
// we couldn't write the error metrics file, um, well, oh well.
}
}

function sendMetrics (metricsFile, metricsRegistry) {
var cliMetrics = JSON.parse(fs.readFileSync(metricsFile))
npm.load({}, function (err) {
if (err) return
npm.registry.config.retry.retries = 0
npm.registry.sendAnonymousCLIMetrics(metricsRegistry, cliMetrics, function (err) {
if (err) {
fs.writeFileSync(path.join(path.dirname(metricsFile), 'last-send-metrics-error.txt'), err.stack)
} else {
fs.unlinkSync(metricsFile)
}
})
})
}
148 changes: 148 additions & 0 deletions test/tap/anon-cli-metrics.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
'use strict'
var path = require('path')
var fs = require('graceful-fs')
var test = require('tap').test
var mr = require('npm-registry-mock')
var Tacks = require('tacks')
var File = Tacks.File
var Dir = Tacks.Dir
var extend = Object.assign || require('util')._extend
var common = require('../common-tap.js')

var basedir = path.join(__dirname, path.basename(__filename, '.js'))
var testdir = path.join(basedir, 'testdir')
var cachedir = path.join(basedir, 'cache')
var globaldir = path.join(basedir, 'global')
var tmpdir = path.join(basedir, 'tmp')
var metricsFile = path.join(cachedir, 'anonymous-cli-metrics.json')

var conf = {
cwd: testdir,
env: extend(extend({}, process.env), {
npm_config_cache: cachedir,
npm_config_tmp: tmpdir,
npm_config_prefix: globaldir,
npm_config_registry: common.registry,
npm_config_metrics_registry: common.registry,
npm_config_loglevel: 'warn'
})
}

var server
var fixture = new Tacks(Dir({
cache: Dir(),
global: Dir(),
tmp: Dir(),
testdir: Dir({
failure: Dir({
'package.json': File({
name: 'failure',
version: '1.0.0',
scripts: {
preinstall: 'false'
}
})
}),
success: Dir({
'package.json': File({
name: 'success',
version: '1.0.0'
})
}),
slow: Dir({
'package.json': File({
name: 'slow',
version: '1.0.0',
scripts: {
preinstall: "node -e 'setTimeout(function(){}, 500)'"
}
})
}),
'package.json': File({
name: 'anon-cli-metrics-test',
version: '1.0.0'
})
})
}))

function setup () {
cleanup()
fixture.create(basedir)
}

function cleanup () {
fixture.remove(basedir)
}

test('setup', function (t) {
setup()
mr({port: common.port, throwOnUnmatched: true}, function (err, s) {
if (err) throw err
server = s
server.filteringPathRegEx(/([/]-[/]npm[/]anon-metrics[/]v1[/]).*/, '$1:id')
server.filteringRequestBody(function (body) {
var metrics = typeof body === 'string' ? JSON.parse(body) : body
delete metrics.from
delete metrics.to
return JSON.stringify(metrics)
})
t.done()
})
})

test('record success', function (t) {
common.npm(['install', '--no-send-metrics', 'success'], conf, function (err, code, stdout, stderr) {
if (err) throw err
t.is(code, 0, 'always succeeding install succeeded')
t.comment(stdout.trim())
t.comment(stderr.trim())
var data = JSON.parse(fs.readFileSync(metricsFile))
t.is(data.metrics.successfulInstalls, 1)
t.is(data.metrics.failedInstalls, 0)
t.done()
})
})

test('record failure', function (t) {
server.put('/-/npm/anon-metrics/v1/:id', {
successfulInstalls: 1,
failedInstalls: 0
}).reply(500, {ok: false})
common.npm(['install', '--send-metrics', 'failure'], conf, function (err, code, stdout, stderr) {
if (err) throw err
t.notEqual(code, 0, 'always failing install fails')
t.comment(stdout.trim())
t.comment(stderr.trim())
var data = JSON.parse(fs.readFileSync(metricsFile))
t.is(data.metrics.successfulInstalls, 1)
t.is(data.metrics.failedInstalls, 1)
t.done()
})
})

test('report', function (t) {
console.log('setup')

server.put('/-/npm/anon-metrics/v1/:id', {
successfulInstalls: 1,
failedInstalls: 1
}).reply(200, {ok: true})
common.npm(['install', '--send-metrics', 'slow'], conf, function (err, code, stdout, stderr) {
if (err) throw err
t.is(code, 0, 'command ran ok')
t.comment(stdout.trim())
t.comment(stderr.trim())
// todo check mock registry for post
var data = JSON.parse(fs.readFileSync(metricsFile))
t.is(data.metrics.successfulInstalls, 1)
t.is(data.metrics.failedInstalls, 0)
t.done()
})
})

test('cleanup', function (t) {
server.close()
cleanup()
t.done()
})

0 comments on commit 190a658

Please sign in to comment.