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.

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

Allow master to add its own metrics #183

Open
miton18 opened this issue Mar 28, 2018 · 8 comments · May be fixed by #280
Open

Allow master to add its own metrics #183

miton18 opened this issue Mar 28, 2018 · 8 comments · May be fixed by #280

Comments

@miton18
Copy link

miton18 commented Mar 28, 2018

Hi !
I work on clustered Node app.
I have some issues when trying to have worker(s) metrics with cluster ones.

Is master process alllowed to expose his own metrics (registered on default Registry) ?

https://github.com/siimon/prom-client/blob/master/lib/cluster.js#L169

@SimenB
Copy link
Collaborator

SimenB commented Mar 28, 2018

I've never used the cluster code, @zbjornson?

@miton18
Copy link
Author

miton18 commented Mar 28, 2018

Is there a way to aggregate clusterMetrics() result with default register.metrics

metricsServer.get('/metrics', (req, res) => {
    aggregatorRegistry.clusterMetrics()
    .then(metrics => {
      res.set('Content-Type', aggregatorRegistry.contentType)
      // How to merge 'metrics' with register.metrics ?
      res.send(metrics))
    })
    .catch(err => {
      error(err)
      return res.status(500).end(err)
    })
  })

@zbjornson
Copy link
Collaborator

@miton18 Since the Prometheus metrics format is plain text, I think that you can just concatenate the string from aggregatorRegistry.clusterMetrics() with the string from masterRegistry.metrics(). From your example (untested):

metricsServer.get('/metrics', (req, res) => {
    aggregatorRegistry.clusterMetrics()
    .then(metrics => {
      res.set('Content-Type', aggregatorRegistry.contentType)
      metrics += otherRegistry.metrics(); // <-- added
      res.send(metrics))
    })
    .catch(err => {
      error(err)
      return res.status(500).end(err)
    })
  })

@miton18
Copy link
Author

miton18 commented Mar 29, 2018

Yes we can... and it's works ;-)

But I asking if it wasn't interesting to natively add Master metrics with workers ones ?

@zbjornson
Copy link
Collaborator

Ah, probably! Happy to review a PR for that.

@brian-brazil
Copy link

I think that you can just concatenate the string from aggregatorRegistry.clusterMetrics() with the string from masterRegistry.metrics().

This will only work if there is no overlap between the metrics, otherwise you could end up with invalid output.

@zbjornson zbjornson changed the title Allow master to add this own metrics Allow master to add its own metrics Jan 31, 2019
@SimenB SimenB linked a pull request Feb 12, 2020 that will close this issue
@vbrvk
Copy link

vbrvk commented Apr 29, 2020

There are any updates? It would be nice to have #280 merged

@zbjornson
Copy link
Collaborator

#280 just need tests. If anyone here can contribute a test, we can get that PR landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants