Skip to content

Merge master registries to cluster metrics (#183)#442

Closed
rsnigel wants to merge 1 commit intosiimon:masterfrom
rsnigel:master
Closed

Merge master registries to cluster metrics (#183)#442
rsnigel wants to merge 1 commit intosiimon:masterfrom
rsnigel:master

Conversation

@rsnigel
Copy link
Copy Markdown

@rsnigel rsnigel commented Jun 10, 2021

Fixes issue #183 by merging the registries configured in the master AggregatorRegistry with the worker registries.
This fixes as well the error that incomplete node cluster metrics are returned, because default metrics collected via collectDefaultMetrics were missing from the master process.
From now on, AggregatorRegistry.setRegistries can be used to configure master process registries as well, that will be merged to the cluster metrics.

@rsnigel rsnigel changed the title Merge master registries with cluster workers (#183) Merge master registries to cluster metrics (#183) Jun 10, 2021
@zbjornson zbjornson mentioned this pull request Aug 4, 2021
@rsnigel
Copy link
Copy Markdown
Author

rsnigel commented Dec 2, 2021

Is there anything missing to merge this PR?

@zbjornson
Copy link
Copy Markdown
Collaborator

Sorry for the delay.

I don't think merging the master registry by default is a good idea because the master is usually unlike the workers, so aggregated metrics could be skewed. (For example, the master probably has a tiny amount of heap space compared to workers.) #280 achieves the same goal as this PR via opt-in behavior, but it needs tests. If you're up for writing tests for it, that would be super helpful!

(Aside, the company I work at doesn't monitor the cluster master at all. The master does so very little that it's not useful to monitor.)

@zbjornson
Copy link
Copy Markdown
Collaborator

Closing per above, but happy to discuss further.

@zbjornson zbjornson closed this Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants