Skip to content
This repository was archived by the owner on Mar 19, 2019. It is now read-only.

Move from statsd-node to node-statsd and use tags. #363

Merged
Natim merged 2 commits into from
Feb 24, 2016
Merged

Move from statsd-node to node-statsd and use tags. #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d146e7c
Move from statsd-node to node-statsd and use tags.
Feb 12, 2016
045eb49
@tarekziade review.
Feb 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion loop/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ module.exports = function(conf, logError, storage, statsdClient) {
var hawkIdHmac = hmac(tokenId, conf.get("hawkIdSecret"));
storage.setHawkSession(hawkIdHmac, authKey, function(err) {
if (statsdClient && err === null) {
statsdClient.count('loop.activated-users', 1);
statsdClient.increment('loop.activated-users');
}
callback(err);
});
Expand Down
2 changes: 1 addition & 1 deletion loop/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ var express = require('express');
var bodyParser = require('body-parser');
var raven = require('raven');
var cors = require('cors');
var StatsdClient = require('statsd-node').client;
var StatsdClient = require('node-statsd');

var PubSub = require('./pubsub');

Expand Down
9 changes: 4 additions & 5 deletions loop/routes/home.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,13 @@ module.exports = function(app, conf, logError, storage, tokBox, statsdClient) {
var pushStatus = (!error && statusCodes.every(isSuccess));
returnStatus(storageStatus, tokboxError, pushStatus, verifierStatus);
if (statsdClient !== undefined) {
statsdClient.count('loop.simplepush.call', 1);
var counter_push_status_counter;
var tag;
if (pushStatus) {
counter_push_status_counter = 'loop.simplepush.call.heartbeat.success';
tag = 'success';
} else {
counter_push_status_counter = 'loop.simplepush.call.heartbeat.failures';
tag = 'failure';
}
statsdClient.count(counter_push_status_counter, 1);
statsdClient.increment('loop.simplepush.call.heartbeat', 1, [tag]);
}
});
});
Expand Down
18 changes: 6 additions & 12 deletions loop/simplepush.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,17 @@ SimplePush.prototype = {
var self = this;

urls.forEach(function(simplePushUrl) {
if (self.statsdClient !== undefined) {
self.statsdClient.count("loop.simplepush.call", 1);
self.statsdClient.count("loop.simplepush.call." + reason, 1);
}
request.put({
url: simplePushUrl,
form: { version: version }
}, function(err) {
var status = 'success';
if (err) {
self.logError(err);
Copy link
Contributor

@n1k0 n1k0 Feb 12, 2016

Choose a reason for hiding this comment

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

Nit: not introduced by this patch by you should probably set the third argument of forEach to instead instead of relying on self in this whole loop.

status = 'failure';
}
if (self.statsdClient !== undefined) {
if (err) {
self.logError(err);
self.statsdClient.count("loop.simplepush.call.failures", 1);
self.statsdClient.count("loop.simplepush.call." + reason + ".failures", 1);
} else {
self.statsdClient.count("loop.simplepush.call.success", 1);
self.statsdClient.count("loop.simplepush.call." + reason + ".success", 1);
}
self.statsdClient.increment("loop.simplepush.call", 1, [reason, status]);
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion loop/tokbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ TokBox.prototype = {
return;
}
if (self.statsdClient !== undefined) {
self.statsdClient.count("loop.tokbox.createSession.count", 1);
self.statsdClient.increment("loop.tokbox.createSession.count");
self.statsdClient.timing(
'loop.tokbox.createSession',
Date.now() - startTime
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"redis": "0.12.1",
"request": "2.45.0",
"sodium": "1.0.13",
"statsd-node": "0.2.3",
"node-statsd": "0.1.1",
"strftime": "0.8.2",
"urlsafe-base64": "1.0.0",
"ws": "1.0.1",
Expand Down
31 changes: 12 additions & 19 deletions test/functional_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ function runOnPrefix(apiPrefix) {
}
});

sandbox.stub(statsdClient, "count");
sandbox.stub(statsdClient, "increment");

supertest(app)
.get(apiPrefix + '/__heartbeat__')
Expand All @@ -406,10 +406,8 @@ function runOnPrefix(apiPrefix) {
'push': false,
'fxaVerifier': true
});
assert.calledTwice(statsdClient.count);
assert.calledWithExactly(statsdClient.count, "loop.simplepush.call", 1);
assert.calledWithExactly(statsdClient.count,
"loop.simplepush.call.heartbeat.failures", 1);
assert.calledOnce(statsdClient.increment);
assert.calledWithExactly(statsdClient.increment, "loop.simplepush.call.heartbeat", 1, ['failure']);
done();
});
});
Expand Down Expand Up @@ -464,7 +462,7 @@ function runOnPrefix(apiPrefix) {
callback(null, {statusCode: 200});
});

sandbox.stub(statsdClient, "count");
sandbox.stub(statsdClient, "increment");

supertest(app)
.get(apiPrefix + '/__heartbeat__')
Expand All @@ -477,11 +475,8 @@ function runOnPrefix(apiPrefix) {
'push': true,
'fxaVerifier': true
});
assert.calledTwice(statsdClient.count);
assert.calledWithExactly(statsdClient.count, "loop.simplepush.call", 1);
assert.calledWithExactly(statsdClient.count,
"loop.simplepush.call.heartbeat.success", 1);

assert.calledOnce(statsdClient.increment);
assert.calledWithExactly(statsdClient.increment, "loop.simplepush.call.heartbeat", 1, ['success']);
done();
});
});
Expand Down Expand Up @@ -778,33 +773,31 @@ function runOnPrefix(apiPrefix) {
});

it("should count new users if the session is created", function(done) {
sandbox.stub(statsdClient, "count");
sandbox.stub(statsdClient, "increment");
supertest(app)
.post(apiPrefix + '/registration')
.type('json')
.send({
'simplePushURL': pushURL
}).expect(200).end(function(err) {
if (err) throw err;
assert.calledOnce(statsdClient.count);
assert.calledOnce(statsdClient.increment);
assert.calledWithExactly(
statsdClient.count,
"loop.activated-users",
1
);
statsdClient.increment,
"loop.activated-users");
Copy link
Contributor

@n1k0 n1k0 Feb 12, 2016

Choose a reason for hiding this comment

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

Nit: this could now fit on its own line.

Copy link
Contributor Author

@Natim Natim Feb 23, 2016

Choose a reason for hiding this comment

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

You mean:

assert.calledWithExactly(
    statsdClient.increment, "loop.activated-users");

Because this doesn't:

assert.calledWithExactly(statsdClient.increment, "loop.activated-users");

done();
});
});

it("shouldn't count a new user if the session already exists",
function(done) {
sandbox.stub(statsdClient, "count");
sandbox.stub(statsdClient, "increment");
jsonReq
.send({
'simple_push_url': pushURL
}).expect(200).end(function(err) {
if (err) throw err;
assert.notCalled(statsdClient.count);
assert.notCalled(statsdClient.increment);
done();
});
});
Expand Down
9 changes: 4 additions & 5 deletions test/fxa_oauth_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,17 +116,16 @@ describe('/fxa-oauth', function () {
});

it("should count new users if the session is created", function(done) {
sandbox.stub(statsdClient, "count");
sandbox.stub(statsdClient, "increment");
supertest(app)
.post(apiPrefix + '/fxa-oauth/params')
.type('json')
.send({}).expect(200).end(function(err) {
if (err) throw err;
assert.calledOnce(statsdClient.count);
assert.calledOnce(statsdClient.increment);
assert.calledWithExactly(
statsdClient.count,
"loop.activated-users",
1
statsdClient.increment,
"loop.activated-users"
Copy link
Contributor

@n1k0 n1k0 Feb 12, 2016

Choose a reason for hiding this comment

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

Same remark.

);
done();
});
Expand Down
22 changes: 8 additions & 14 deletions test/simplepush_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,14 @@ describe("simplePush object", function() {
});

it("should notify using the statsd client if present", function() {
var statsdClient = { count: function() {} };
var statsdSpy = sandbox.spy(statsdClient, "count");
var statsdClient = { increment: function() {} };
var statsdSpy = sandbox.spy(statsdClient, "increment");

var simplePush = new SimplePush(statsdClient);
simplePush.notify("reason", "url1", 12345);

assert.callCount(statsdSpy, 4);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.reason", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.success", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.reason.success", 1);
assert.calledOnce(statsdSpy);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call", 1, ["reason", "success"]);
});

it("should notify using the statsd client for errors if present", function() {
Expand All @@ -78,16 +75,13 @@ describe("simplePush object", function() {
callback("error");
});

var statsdClient = { count: function() {} };
var statsdSpy = sandbox.spy(statsdClient, "count");
var statsdClient = { increment: function() {} };
var statsdSpy = sandbox.spy(statsdClient, "increment");

var simplePush = new SimplePush(statsdClient);
simplePush.notify("reason", "url1", 12345);

assert.callCount(statsdSpy, 4);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.reason", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.failures", 1);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call.reason.failures", 1);
assert.calledOnce(statsdSpy);
assert.calledWithExactly(statsdSpy, "loop.simplepush.call", 1, ["reason", "failure"]);
});
});
4 changes: 2 additions & 2 deletions test/tokbox_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ describe("TokBox", function() {
var tokBox, openTokSpy, statsdClient, statsdTimer, statsdCount;

beforeEach(function() {
statsdClient = { timing: function() {}, count: function() {} };
statsdClient = { timing: function() {}, increment: function() {} };
statsdTimer = sandbox.spy(statsdClient, "timing");
statsdCount = sandbox.spy(statsdClient, "count");
statsdCount = sandbox.spy(statsdClient, "increment");

openTokSpy = sandbox.spy(loopTokbox, "OpenTok");
openTokSpy.withArgs(
Expand Down