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

Make prometheus-scrapable #861

Closed
erik-stephens opened this issue Oct 18, 2018 · 6 comments
Closed

Make prometheus-scrapable #861

erik-stephens opened this issue Oct 18, 2018 · 6 comments
Assignees

Comments

@erik-stephens
Copy link
Contributor

We'd like to be able to have prometheus scrape teraslice. It needs to publish metrics in exposition format [1].

Implementation idea: make existing /cluster/stats endpoint switch its output format based on Accept header like application/openmetrics-text; version=0.0.1,text/plain;version=0.0.4;q=0.5,*/*;q=0.1 [2], defaulting to its current json format. In other words, no need to touch all the points where the metrics are collected.

[1] https://prometheus.io/docs/instrumenting/exposition_formats/
[2] https://github.com/prometheus/prometheus/blob/a30348f1a49f76b11efbf5cc2fe1b0bdb06a6cdb/scrape/scrape.go#L454

@godber godber self-assigned this Oct 18, 2018
@godber godber mentioned this issue Oct 18, 2018
@godber
Copy link
Member

godber commented Oct 19, 2018

I've looked at incorporating this change with the API changes I am making today but adding it can open a can of worms that I don't want to open yet. There's a decent node library for this, prom-client. It handles storing the metrics as well, which is something I don't want to touch right now. I looked further to see if I could just use the code that generates the output but it's tightly coupled with it's metric registry.

We could generate our own text ... but I think the right approach would be to try and fully integrate with the prom-client module and use it.

So, I'm punting on this for now.

@godber godber removed their assignment Oct 19, 2018
@godber
Copy link
Member

godber commented Oct 19, 2018

Oh, also here's where I got WRT handling the accept header:

    v1routes.get('/cluster/stats', (req, res) => {
        logger.trace('GET /cluster/stats endpoint has been called');
        const acceptHeader = _.get(req, 'headers.accept', '');
        const stats = executionService.getClusterStats();

        if ((acceptHeader !== '') && (acceptHeader.indexOf('application/openmetrics-text;') > -1)) {
            res.status(200).send(stats);
        } else {
            // for backwards compatability (unsupported for prometheus)
            stats.slicer = stats.controllers;
            res.status(200).json(stats);
        }
    });

@godber godber self-assigned this Oct 19, 2018
@godber
Copy link
Member

godber commented Oct 19, 2018

I take it back, I'll bolt on something simple temporarily.

godber added a commit that referenced this issue Oct 19, 2018
@godber
Copy link
Member

godber commented Oct 22, 2018

This is done and hopefully works with prometheus correctly:

curl -Ss -H "Accept: fubar application/openmetrics-text; mcfoo face" localhost:5678/cluster/stats
# TYPE teraslice_slices_processed counter
0
# TYPE teraslice_slices_failed counter
0
# TYPE teraslice_slices_queued counter
0
# TYPE teraslice_workers_joined counter
0
# TYPE teraslice_workers_disconnected counter
0
# TYPE teraslice_workers_reconnected counter
0

@godber
Copy link
Member

godber commented Oct 22, 2018

As shown above, it looks only for the application/openmetrics-text part of the Accept header. If it doesn't work we'll file a bug.

@godber godber closed this as completed Oct 22, 2018
@godber
Copy link
Member

godber commented Nov 15, 2018

Note that the application/openmetrics-text Accept header is not sent until more recent prometheus releases. It's present in v2.5.0 but not in v2.3.1.

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

No branches or pull requests

2 participants