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: avoid mutation bug in registry #273

Merged
merged 3 commits into from
Jun 20, 2019
Merged

Conversation

Raynos
Copy link
Contributor

@Raynos Raynos commented Jun 15, 2019

The defaultLabels feature is buggy when used in combination
with the .labels() sub counter feature.

We mutate the object in the serialization implementation which
means the next time you observe the value the labels object has
been incorrectly mutated.

The functionality of .labels() is supposed to be the caching
of the labels object itself instead of constantly re-using it.

I may actually be the only one using the caching feature of the
.labels() method which is why this bug was not caught earlier.

The .labels() method is not broken if you use it as

histogram.labels(...).observe(int)

But is broken if you do

class Foo {
    constructor() {
        this.myHistogram = histogram.labels(...)
    }

    method() {
        this.myHistogram.observe(int)
    }
}

The defaultLabels feature is buggy when used in combination
with the `.labels()` sub counter feature.

We mutate the object in the serialization implementation which
means the next time you observe the value the labels object has
been incorrectly mutated.

The functionality of `.labels()` is supposed to be the caching
of the labels object itself instead of constantly re-using it.
Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Could you add a test and update the changelog as well, please?

Code itself LGTM 🙂

@Raynos
Copy link
Contributor Author

Raynos commented Jun 20, 2019

I've updated changelog and added a test that fails without my patch.

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@SimenB SimenB merged commit 2c0020f into siimon:master Jun 20, 2019
@SimenB
Copy link
Collaborator

SimenB commented Jun 20, 2019

@Raynos
Copy link
Contributor Author

Raynos commented Jun 20, 2019

Awesome.

@Raynos Raynos deleted the fix-mutation-bug branch June 20, 2019 12:27
doochik added a commit to doochik/prom-client that referenced this pull request Sep 20, 2019
Bug introduced at siimon#220 and fixed for .getMetricAsPrometheusString() at siimon#273
doochik added a commit to doochik/prom-client that referenced this pull request Nov 13, 2019
Bug introduced at siimon#220 and fixed for .getMetricAsPrometheusString() at siimon#273
zbjornson pushed a commit that referenced this pull request Nov 13, 2019
Bug introduced at #220 and fixed for .getMetricAsPrometheusString() at #273
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.

2 participants