Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telemetry #302

Merged
merged 18 commits into from Dec 20, 2016
Merged
5 changes: 4 additions & 1 deletion .eslintrc
@@ -1,3 +1,6 @@
{
"extends": "airbnb/legacy"
"extends": "airbnb/legacy",
"rules": {
"max-len": ["error", 100, { "ignoreComments": true }]
}
}
15 changes: 9 additions & 6 deletions lib/clients/client.js
Expand Up @@ -14,6 +14,7 @@ var SlackAPIError = require('./errors').SlackAPIError;
var callTransport = require('./transports/call-transport');
var getLogger = require('../helpers').getLogger;
var helpers = require('./helpers');
var globalHelpers = require('../helpers');
var requestsTransport = require('./transports/request').requestTransport;
var retryPolicies = require('./retry-policies');

Expand All @@ -23,7 +24,6 @@ var retryPolicies = require('./retry-policies');
* @param {string} token The Slack API token to use with this client.
* @param {Object} opts
* @param {String} opts.slackAPIUrl The Slack API URL.
* @param {String} opts.userAgent The user-agent to use, defaults to node-slack.
* @param {Function} opts.transport Function to call to make an HTTP call to the Slack API.
* @param {string=} opts.logLevel The log level for the logger.
* @param {Function=} opts.logger Function to use for log calls, takes (logLevel, logString) params.
Expand All @@ -49,9 +49,6 @@ function BaseAPIClient(token, opts) {
/** @type {Function} */
this.transport = clientOpts.transport || requestsTransport;

/** @type {string} */
this.userAgent = clientOpts.userAgent || 'node-slack';

/**
* Default to attempting 5 retries within 5 minutes, with exponential backoff.
*/
Expand All @@ -78,7 +75,6 @@ function BaseAPIClient(token, opts) {

inherits(BaseAPIClient, EventEmitter);


BaseAPIClient.prototype.emit = function emit() {
BaseAPIClient.super_.prototype.emit.apply(this, arguments);
this.logger('debug', arguments);
Expand Down Expand Up @@ -138,7 +134,14 @@ BaseAPIClient.prototype._callTransport = function _callTransport(task, queueCb)
*/
BaseAPIClient.prototype._makeAPICall = function _makeAPICall(endpoint, apiArgs, apiOptArgs, optCb) {
var apiCallArgs = helpers.getAPICallArgs(
this._token, this.userAgent, this.slackAPIUrl, endpoint, apiArgs, apiOptArgs, optCb);
this._token,
globalHelpers.getVersionString(),
this.slackAPIUrl,
endpoint,
apiArgs,
apiOptArgs,
optCb
);
var cb = apiCallArgs.cb;
var args = apiCallArgs.args;
var promise;
Expand Down
6 changes: 3 additions & 3 deletions lib/clients/rtm/client.js
Expand Up @@ -36,7 +36,6 @@ var SlackRTMSendError = require('../errors').SlackRTMSendError;
var makeMessageEventWithSubtype = require('../events/utils').makeMessageEventWithSubtype;
var wsSocketFn = require('../transports/ws');


/**
*
* @param {String} token
Expand Down Expand Up @@ -518,7 +517,7 @@ RTMClient.prototype._handleMessageAck = function handleMessageAck(replyTo, messa
} else {
// This should be impossible. If the message is received with no message type, it's a response
// to a message sent by this client, so the absence of a handler for it should never happen.
this.logger('error', 'message received with unknown reply_to: ' + message);
this.logger('error', 'message received with unknown reply_to: ' + JSON.stringify(message));
}
};

Expand Down Expand Up @@ -556,7 +555,8 @@ RTMClient.prototype._handleMostRecentMsgReply = function _handleMostRecentMsgRep
this._handleMsgResponse(replyTo, null, msg);
} else {
// The only time this should happen is when a client first connects
this.logger('info', 'message received on reconnect with no registered callback:\n' + message);
this.logger('info', 'message received on reconnect with no registered callback:\n'
+ JSON.stringify(message));
}

pendingMessageIds = keys(this._pendingMessages);
Expand Down
6 changes: 5 additions & 1 deletion lib/clients/transports/ws.js
Expand Up @@ -4,7 +4,7 @@

var HttpsProxyAgent = require('https-proxy-agent');
var WebSocket = require('ws');

var globalHelpers = require('../../helpers');

/**
*
Expand All @@ -23,6 +23,10 @@ var wsTransport = function wsTransport(socketUrl, opts) {
wsOpts.agent = new HttpsProxyAgent(proxyURL);
}

wsOpts.headers = {
'User-Agent': globalHelpers.getVersionString()
};

return new WebSocket(socketUrl, wsOpts);
};

Expand Down
37 changes: 37 additions & 0 deletions lib/helpers.js
Expand Up @@ -5,6 +5,8 @@
var ConsoleTransport = require('winston').transports.Console;
var bind = require('lodash').bind;
var winston = require('winston');
var os = require('os');
var pkginfo = require('pkginfo')(module, 'version', 'repository'); // eslint-disable-line no-unused-vars


/**
Expand All @@ -21,5 +23,40 @@ var getLogger = function getLogger(optLogLevel, optTransport) {
return bind(logger.log, logger);
};

// TODO we can do this better
var versionStr = module.exports.repository.replace('/', ':') + '/' + module.exports.version + ' '
+ os.platform() + '/' + os.release() + ' '
+ 'node/' + process.version.replace('v', '');

var getVersionString = function getVersionString() {
return versionStr;
};

/**
*
* A function to append information to the end of the informational string. You can append an
* arbitrary string in the first parameter (although all `/` characters will be replaced with
* `:` characters. You can provide an optional string for the version of the resource as well, when
* this makes sense.
*
* In general, you should use this function to let us know what you are building with this SDK. If
* you are building a bot called "FooBot", and are currently on version "1.0", then call the function
* as `appendToVersionString('FooBot', '1.0') to have that information sent to Slack during
* interactions with the Slack Platform
*
* @param {string} name
* @param {string} [version]
*
*/
var appendToVersionString = function appendToVersionString(name, version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put some JSDoc on this function so that its surfaced by our reference docs? the intention is for this to be used by "higher-level" users of the SDK, right?

var correctName = name.replace('/', ':');
if (version) {
versionStr = versionStr + ' ' + correctName + '/' + version.replace('/', ':');
} else {
versionStr = versionStr + ' ' + correctName;
}
};

module.exports.getLogger = getLogger;
module.exports.getVersionString = getVersionString;
module.exports.appendToVersionString = appendToVersionString;
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -28,6 +28,7 @@
"https-proxy-agent": "^1.0.0",
"inherits": "^2.0.1",
"lodash": "^4.13.1",
"pkginfo": "^0.4.0",
"request": "^2.64.0",
"retry": "^0.9.0",
"url-join": "0.0.1",
Expand Down
3 changes: 2 additions & 1 deletion test/.eslintrc
Expand Up @@ -9,6 +9,7 @@
"no-var": 0,
"object-shorthand": 0,
"padded-blocks": 0,
"func-names": 0
"func-names": 0,
"max-len": ["error", 100, { "ignoreComments": true }]
}
}
1 change: 0 additions & 1 deletion test/clients/helpers.js
Expand Up @@ -87,6 +87,5 @@ describe('Client Helpers', function () {
expect(args.cb).to.equal(null);
expect(args.args.data).to.deep.equal({ test: '1', token: 't' });
});

});
});
3 changes: 1 addition & 2 deletions test/clients/web/client.js
Expand Up @@ -4,6 +4,7 @@ var path = require('path');
var lodash = require('lodash');
var nock = require('nock');
var sinon = require('sinon');
var pkginfo = require('pkginfo')(module, 'version', 'repository'); // eslint-disable-line no-unused-vars

var WebAPIClient = require('../../../lib/clients/web/client');
var retryPolicies = require('../../../lib/clients/retry-policies');
Expand Down Expand Up @@ -42,13 +43,11 @@ describe('Web API Client', function () {
it('should accept supplied defaults when present', function () {
var opts = {
slackAPIUrl: 'test',
userAgent: 'test',
transport: lodash.noop
};
var client = new WebAPIClient('test-token', opts);

expect(client.slackAPIUrl).to.equal('test');
expect(client.userAgent).to.equal('test');
});

it('should register facets during construction', function () {
Expand Down
46 changes: 46 additions & 0 deletions test/helpers.js
@@ -0,0 +1,46 @@
var expect = require('chai').expect;
var helpers = require('../lib/helpers');
var os = require('os');
var pkginfo = require('pkginfo')(module, 'version', 'repository'); // eslint-disable-line no-unused-vars

var userAgent = module.exports.repository.replace('/', ':') + '/' + module.exports.version
+ ' ' + os.platform() + '/' + os.release()
+ ' node/' + process.version.replace('v', '');

describe('Version strings', function () {
describe('getVersionString', function () {
it('returns the current version, when unmodified', function () {
var str = helpers.getVersionString();
expect(str).to.equal(userAgent);
});
});

describe('appendToVersionString', function () {
it('appends a new value', function () {
var str;
helpers.appendToVersionString('foobar', '1.2');
str = helpers.getVersionString();
expect(str).to.equal(userAgent + ' foobar/1.2');
});
});

describe('appendToVersionString', function () {
it('should never overwrite an existing value', function () {
var str;
helpers.appendToVersionString('foobar', '1.3');
str = helpers.getVersionString();
// TODO that the order of these tests _matters_ bothers me
expect(str).to.equal(userAgent + ' foobar/1.2 foobar/1.3');
});
});

describe('appendToVersionStringWithoutVersion', function () {
it('should never overwrite an existing value', function () {
var str;
helpers.appendToVersionString('foobar');
str = helpers.getVersionString();
// TODO that the order of these tests _matters_ bothers me
expect(str).to.equal(userAgent + ' foobar/1.2 foobar/1.3 foobar');
});
});
});