Msiebuhr/add statsd to stream refactor #15
Conversation
stephen-palmer
left a comment
stephen-palmer
left a comment
There was a problem hiding this comment.
Thanks for the contribution - I'm agreeable to adding statsd support but there's some work to do yet:
I don't like that there is no execution path without statsd, which will by far be the most common use case. An alternative design to consider would be to add events to the remaining command handlers (_onTransactionEnd already emits an event, for instance) and then setup an event listener object that consolidates all of the statsd logic, which will only be registered if statsd is enabled.
Also, this will need test coverage before being accepted. (except for main.js which doesn't really have tests)
| throw new Error("Invalid transaction isolation"); | ||
| } | ||
|
|
||
| if (this.transactionTimer) { |
There was a problem hiding this comment.
This should be moved after the 'await' on endPutTransaction below.
| let server = null; | ||
|
|
||
| let cacheOpts = {}; | ||
| let cacheOpts = { |
There was a problem hiding this comment.
Why pass the statsd object to the cache init() if it isn't used in the cache module?
|
Closing due to inactivity. |
3 participants
First take at adding statsd-metrics to streaming server.
Reference to PR on the "old" server: #11