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

Fix histogram scrape performance #219

Merged
merged 3 commits into from Sep 19, 2018
Merged

Conversation

nowells
Copy link
Contributor

@nowells nowells commented Sep 19, 2018

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.
lib/histogram.js Outdated
@@ -288,7 +288,7 @@ function extractBucketValuesForExport(histogram) {

function addSumAndCountForExport(histogram) {
return (acc, d) => {
acc = acc.concat(d.buckets);
acc.push.apply(acc, d.buckets);
Copy link
Collaborator

@SimenB SimenB Sep 19, 2018

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
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
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
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
Copy link
Collaborator

SimenB commented Sep 19, 2018

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

@nowells
Copy link
Contributor Author

nowells commented Sep 19, 2018

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

Done

@SimenB SimenB merged commit 45e0c3e into siimon:master Sep 19, 2018
@nowells
Copy link
Contributor Author

nowells commented Sep 19, 2018

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
Copy link
Collaborator

SimenB commented Sep 19, 2018

It's released as 11.1.2 now 🙂

@nowells
Copy link
Contributor Author

nowells commented Sep 19, 2018

It's released as 11.1.2 now 🙂

❤️ thanks!!!

@siimon
Copy link
Owner

siimon commented Sep 20, 2018

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.

None yet

3 participants