Fix telemetry header #362
Fix telemetry header #362
Conversation
src/management/index.js
Outdated
| @@ -299,26 +299,15 @@ var ManagementClient = function(options) { | |||
| * @return {Object} Object containing client information. | |||
| */ | |||
| ManagementClient.prototype.getClientInfo = function() { | |||
| var clientInfo = { | |||
| return { | |||
There was a problem hiding this comment.
Seems like it would be a good idea to pull this and AuthenticationClient.prototype.getClientInfo from the same place...
There was a problem hiding this comment.
I can extract a new file/class and reference it from both places. Initially I thought on adding this header on a common place as well, instead of having each "set of endpoints module" set it. But I don't want to make a huge refactor of this. I know the work for auto-generating the APIs is eventually going to replace all.
src/auth/index.js
Outdated
| name: 'node-auth0', | ||
| version: pkg.version, | ||
| dependencies: [], | ||
| environment: [ | ||
| env: [ |
There was a problem hiding this comment.
env should not be an array, should be an object. See the RFC 👈 Auth0 internal link
|
|
||
| it('should configure instances with default telemetry header', function() { | ||
| sinon.stub(AuthenticationClient.prototype, 'getClientInfo', function() { | ||
| return { name: 'test-sdk', version: 'ver-123' }; |
There was a problem hiding this comment.
Why was env left out of this test object?
There was a problem hiding this comment.
This is just mocking a hardcoded response we can compare against in the tests. In order to avoid depending on versions numbers that can change, etc.
But the default implementation is already being tested here https://github.com/auth0/node-auth0/pull/362/files/ebe7db56674569acdfbcb25458fbe2357e9728ca#diff-16d97c0f0c5a6d9cc470e15de5e52580R72
| 'Auth0-Client': 'eyJuYW1lIjoidGVzdC1zZGsiLCJ2ZXJzaW9uIjoidmVyLTEyMyJ9', | ||
| 'Content-Type': 'application/json' | ||
| }; | ||
| expect(client.oauth.oauth.options.headers).to.contain(requestHeaders); |
| }); | ||
|
|
||
| it('should configure instances with custom telemetry header', function() { | ||
| var customTelemetry = { name: 'custom', version: 'beta-01', env: { node: 'v10' } }; |
There was a problem hiding this comment.
Not to open a can of worms here ... but someone can just pass whatever and it goes through? What about telemetry without a name?
There was a problem hiding this comment.
Yes. They can. I think the decision for other SDKs was to prevent the telemetry being sent at all if the name was not present, should I do that here as well?
There was a problem hiding this comment.
@lbalmaceda - Seems like a good idea to me. Then at least we can filter some of the garbage that might be sent here.
| * | ||
| * @return {Object} Object containing client information. | ||
| */ | ||
| utils.generateClientInfo = function() { |
There was a problem hiding this comment.
new utils method to generate consistent telemetry across different api clients
| var telemetry = jsonToBase64(options.clientInfo || this.getClientInfo()); | ||
| managerOptions.headers['Auth0-Client'] = telemetry; | ||
| var clientInfo = options.clientInfo || generateClientInfo(); | ||
| if ('string' === typeof clientInfo.name && clientInfo.name.length > 0) { |
There was a problem hiding this comment.
if name is empty or not present, skip sending telemetry
| @@ -67,7 +67,8 @@ var ManagementTokenProvider = function(options) { | |||
| domain: this.options.domain, | |||
| clientId: this.options.clientId, | |||
| clientSecret: this.options.clientSecret, | |||
| telemetry: this.options.telemetry | |||
| telemetry: this.options.telemetry, | |||
| clientInfo: this.options.clientInfo | |||
There was a problem hiding this comment.
this class creates an AuthenticationClient for itself. I'm just passing down the telemetry customizations to that instance
lbalmaceda
force-pushed
the
update-telemetry
branch
from
b20d58c
to
7ecdf16
Compare
Changes
Testing
Added unit tests asserting the header is being set, customized and removed when asked so.
Checklist