Skip to content

Fix histogram scrape performance#219

Merged
SimenB merged 3 commits into
prometheus:masterfrom
nowells:histogram-performance
Sep 19, 2018
Merged

Fix histogram scrape performance#219
SimenB merged 3 commits into
prometheus:masterfrom
nowells:histogram-performance

Conversation

@nowells

@nowells nowells commented Sep 19, 2018

Copy link
Copy Markdown
Contributor

When profiling histogram scraping I discovered that the major bottleneck is creating a clone of the accumulator in reduce. We should not actually need to skip mutation.

You can see the benchmark of concat vs push here: https://codeburst.io/jsnoob-push-vs-concat-basics-and-performance-comparison-7a4b55242fa9

This should address #216

I used this test script to profile.

const prom = require('prom-client');
const _ = require('lodash');

const histogram = new prom.Histogram({
    name: 'histogram',
    help: 'histogram',
    labelNames: ['a', 'b', 'c', 'd'],
});

_.times(2, a => {
    _.times(3, b => {
        _.times(10, c => {
            _.times(100, d => {
                const end = histogram.startTimer();

                end({a,b,c,d});
            });
        });
    });
});

console.log('Starting metrics');
const now = Date.now();
const metrics = prom.register.getMetricsAsJSON();

console.log(`${(Date.now() - now) / 1000}s`);

Which resulted in before this patch of 7.021s and after of 0.501s

When profiling histogram scraping I discovered that the major
bottleneck is creating a clone of the accumulator in reduce.
We should not actually need to skip mutation.
Comment thread lib/histogram.js Outdated
function addSumAndCountForExport(histogram) {
return (acc, d) => {
acc = acc.concat(d.buckets);
acc.push.apply(acc, d.buckets);

@SimenB SimenB Sep 19, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not just acc.push(d.buckets);?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

oh, it's an array. makes sense :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it can be acc.push(...d.buckets);, we have a minimum node version of 6

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I didn't look up minimum node version, I had it that way initially but then decided to start off with more support. I'll update to the ES6 syntax.

@SimenB

SimenB commented Sep 19, 2018

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Mind updating the changelog as well?

@nowells

nowells commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the PR! Mind updating the changelog as well?

Done

@SimenB SimenB merged commit 45e0c3e into prometheus:master Sep 19, 2018
@nowells

nowells commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the merge @SimenB! I did not package bump because I assume that you do that as part of your release process. Thanks for the great package, and for such a quick response!

@SimenB

SimenB commented Sep 19, 2018

Copy link
Copy Markdown
Collaborator

It's released as 11.1.2 now 🙂

@nowells

nowells commented Sep 19, 2018

Copy link
Copy Markdown
Contributor Author

It's released as 11.1.2 now 🙂

❤️ thanks!!!

@siimon

siimon commented Sep 20, 2018

Copy link
Copy Markdown
Contributor

So nice!! Thank you! ❤️

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.

3 participants