Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added opportunity add any registry to merge with clusters #280

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added opportunity add any registry to merge with clusters #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ export class AggregatorRegistry extends Registry {
* Gets aggregated metrics for all workers. The optional callback and
* returned Promise resolve with the same value; either may be used.
* @param {Function?} cb (err, metrics) => any
* @param {Object?} opt options for
* @param {Registry?} opt.registry additional registry to merge with
* @return {Promise<string>} Promise that resolves with the aggregated
* metrics.
*/
Expand Down
14 changes: 12 additions & 2 deletions lib/cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const GET_METRICS_REQ = 'prom-client:getMetricsReq';
const GET_METRICS_RES = 'prom-client:getMetricsRes';

let registries = [Registry.globalRegistry];
let additionRegistry;
let requestCtr = 0; // Concurrency control
let listenersAdded = false;
const requests = new Map(); // Pending requests for workers' local metrics.
Expand All @@ -31,12 +32,16 @@ class AggregatorRegistry extends Registry {
* Gets aggregated metrics for all workers. The optional callback and
* returned Promise resolve with the same value; either may be used.
* @param {Function?} callback (err, metrics) => any
* @param {Object} opt options for
* @param {Registry} opt.registry additional registry to merge with
* @return {Promise<string>} Promise that resolves with the aggregated
* metrics.
*/
clusterMetrics(callback) {
clusterMetrics(callback, opt) {
const requestId = requestCtr++;

if (opt && opt.registry) {
additionRegistry = opt.registry;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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).

}
return new Promise((resolve, reject) => {
function done(err, result) {
// Don't resolve/reject the promise if a callback is provided
Expand Down Expand Up @@ -96,6 +101,11 @@ class AggregatorRegistry extends Registry {
const aggregatedRegistry = new Registry();
const metricsByName = new Grouper();

// add addition registry such as master
if (additionRegistry) {
metricsArr.push(additionRegistry.getMetricsAsJSON());
}

// Gather by name
metricsArr.forEach(metrics => {
metrics.forEach(metric => {
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.