Added opportunity add any registry to merge with clusters #280
Conversation
|
@zbjornson do you have any opinions? |
|
This will fix #183 👍 and I think is the correct approach. Could you add a test case for it and update the changelog please? |
zbjornson
force-pushed
the
master
branch
2 times, most recently
from
d6f2cf6
to
29b6d86
Compare
|
Still waiting for a test before merging this one |
| const requestId = requestCtr++; | ||
|
|
||
| if (opt && opt.registry) { | ||
| additionRegistry = opt.registry; |
There was a problem hiding this comment.
I might be missing something here as I'm new to this codebase, but it seems odd to call an instance method (anAggregatorRegistryInstance.clusterMetrics()) which has the side effect of affecting a static method (AggregatorRegistry.aggregate()). There is an implicit dependency here in that the execution path will ultimately call AggregatorRegistry.aggregate() with the hope the right additionRegistry is still set.
Perhaps this additionalRegistry should be stored with the inflight request (perhaps by modifying the shape of data stored in requests) state so it's metrics can later be retrieved before the call to AggregatorRegistry.aggregate().
Again I'm pretty new to looking at this, so forgive me if this is off base.
There was a problem hiding this comment.
(16 months later)
Fair point, that would be a good improvement.
We already have module-level state with AggregatorRegistry.setRegistries(). That should maybe be changed also (in a separate PR).
Hello. We used cluster in project. Master process make logics to exclusively write to db extra data and used metrics too. This commit allow to add master repository to merge with clusters optionals.