refactor(server): remove separate notifier process #1800
Conversation
| @@ -150,7 +151,7 @@ Lug.prototype.notifyAttachedServices = function (name, request, data) { | |||
| data: unbuffer(data) | |||
| } | |||
| e.data.metricsContext = metricsContextData | |||
| this.stdout.write(JSON.stringify(e) + '\n') | |||
| this.notifier.send(e) | |||
|
@jrgm r? @mozilla/fxa-devs r? |
There was a problem hiding this comment.
@vladikoff Looks good, just a couple things. Noticed that key_server.js logs to stdout. Should it be updated to emit sns event?
lib/notifier.js
Outdated
| const AWS = require('aws-sdk') | ||
| const config = require('../config') | ||
|
|
||
| const snsTopicArn = config.get('snsTopicArn') |
There was a problem hiding this comment.
The config value name snsTopicArn doesn't mention what the topic is used for. Maybe notifierSnsTopicArn?
There was a problem hiding this comment.
That will probably require changes to existing configs in puppet, etc. So I'd rather just leave it alone, I can rename the local variable though
test/local/notifier.js
Outdated
| it('works with sns configuration', () => { | ||
| const config = { | ||
| get: (key) => { | ||
| if(key === 'snsTopicArn') { |
test/local/notifier.js
Outdated
| it('works with disabled configuration', () => { | ||
| const config = { | ||
| get: (key) => { | ||
| if(key === 'snsTopicArn') { |
lib/notifier.js
Outdated
| Message: JSON.stringify(msg) | ||
| }, (err, data) => { | ||
| if (err) { | ||
| log.error({op: 'Notifier.sent', err: err}) |
There was a problem hiding this comment.
Previous file used Notifier.published, do we have any graphs or metrics that would need updating to use Notifier.sent?
There was a problem hiding this comment.
Good idea. reverting to the older name...
|
Updated |
cc @jrgm
Work in progress...Fixes #1796