Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Stats: perform a deepcopy of bucketboundaries when performing measurements #222

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

mayurkale22
Copy link
Member

@mayurkale22 mayurkale22 commented Dec 6, 2018

While doing measurements, we were reusing bucket boundaries(counts and buckets)
from pervious record instead of creating a new copy like other libraries do.

Fixes #221

Copy link
Member

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for the fix @mayurkale22, LGTM!

@odeke-em
Copy link
Member

odeke-em commented Dec 6, 2018

Although could you please update the title to stats: perform a deepcopy of bucketboundaries when performing measurements

@mayurkale22
Copy link
Member Author

Although could you please update the title to stats: perform a deepcopy of bucketboundaries when performing measurements

Make sense, Done.

@mayurkale22 mayurkale22 changed the title Support record with measurements in succession from a single view and single measure Stats: perform a deepcopy of bucketboundaries when performing measurements Dec 6, 2018
Copy link
Contributor

@justindsmith justindsmith left a comment

Choose a reason for hiding this comment

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

one small nit, but otherwise looks good!

one other nit, this technically isn't a deep copy, since Object.assign does a shallow copy. In this case it doesn't really matter because both of the arrays are flat number arrays, but just wanted to call it out so it's not confusing later on.

view.recordMeasurement(measurement1);
}

it('should has points', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "should have points"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Corrected typos in other existing tests.

…ments

While doing measurements, we were reusing bucket boundaries(counts and buckets)
from pervious record instead of creating a new copy like other libraries do.

Fixes census-instrumentation#221
@mayurkale22
Copy link
Member Author

one other nit, this technically isn't a deep copy, since Object.assign does a shallow copy. In this case it doesn't really matter because both of the arrays are flat number arrays, but just wanted to call it out so it's not confusing later on.

I agreed, thanks for the explanation.

@mayurkale22 mayurkale22 merged commit fa41519 into census-instrumentation:master Dec 10, 2018
@mayurkale22 mayurkale22 deleted the bug_fix branch December 10, 2018 06:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants