-
Notifications
You must be signed in to change notification settings - Fork 8
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
Track origin event #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits: some "clean code" request (up to your judgement not required)
variable name changes I would like to see
headers: {} | ||
} | ||
const ns = getNamespace('ponos') | ||
jobMeta.headers.previousEvent = ns && ns.get('previousEvent') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code copied twice, can you make little helper function
_getNamespaceValueOrNull {return ns && ns.get('previousEvent')}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here you can convert currentWorkerName
to sendingWorkerName
or publisherWorkerName
or publisherName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
* @param {Object} content Content to send. | ||
* @return {Object} payload with `jobBuffer` and `jobMeta` props | ||
*/ | ||
static buildPayload (content: Object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm actually, this function is doing 2 independent things, building payload and building data. split out into 2 nice small functions? getPayload
getHeaders
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -133,6 +133,7 @@ class Worker { | |||
return Promise.fromCallback((cb) => { | |||
cls.run(() => { | |||
cls.set('tid', this.tid) | |||
cls.set('previousEvent', this.queue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking harder about it in this context I dont think previousEvent
is the right word here. It seems more like currentWorkerName
would be better here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that one is better. I didn't like event
part, because event can be published from task. So WorkerName
is better than event. current
also makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like just WorkerName
since in the scope of the function there is no next
it is always current.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but either is fine
timestamp: Date.now() | ||
} | ||
this.log.info({ event: exchange, job: content, jobMeta: jobMeta }, 'Publishing event') | ||
this._validatePublish(exchange, content, 'events') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems like you can break this and above changes to a helper function as well. also I would not write unit test for helper functions, just keep the current ones passing and coverage will tell us if we need to write another test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what to extract here into the helper function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(name, content, type)
this._validatePublish(name, content, 'tasks')
const payload = RabbitMQ.buildJobPayload(content)
const meta = RabbitMQ.buildJobMeta(this.name)
this.log.info({ queue: name, job: content, jobMeta: meta }, `Publishing ${name}`)
this._incMonitor(type, name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then you can use this here and here:
https://github.com/Runnable/ponos/pull/99/files#diff-2457f035d00feb69f8f52297277627edR273
jobMeta
now hasheaders.previousEvent
with name of the event that triggered current event.