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.

Sign up for GitHub

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

Jump to bottom

Bug 1716952 - Implement the Memory Distribution Metric #1514

Merged
merged 10 commits into from
Oct 6, 2022
Merged

Bug 1716952 - Implement the Memory Distribution Metric #1514

merged 10 commits into from
Oct 6, 2022

Conversation

rosahbruno
Copy link
Contributor

@rosahbruno rosahbruno commented Sep 20, 2022 •
edited
Loading

Implements the memory distribution metric

Notable changes

  • Ported over all the functionality for the memory distribution metric
  • Ported over all the tests for memory distributions
  • Created distributions helper file containing types/functions for our distributions
  • Created MemoryUnit enum and conversion helper
  • Wrote additional tests for memory distributions
  • Added the new metric to the QT entry file
  • Updated global schema test to include the new memory distribution metric
  • Updated metrics.yaml to include data for new memory distribution metric
  • fixed some typos

PR for updating the Glean Book docs - mozilla/glean#2203

Pull Request checklist

  • Quality: Make sure this PR builds and runs cleanly.
    • Inside the glean/ folder, run:
      • npm run test Runs all tests
      • npm run lint Runs all linters
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
  • Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work

rosahbruno added a commit to rosahbruno/glean.js that referenced this pull request Sep 20, 2022
@rosahbruno
Update changelog for mozilla#1514
c71eaed
@rosahbruno rosahbruno changed the base branch from main to release September 21, 2022 12:58
@rosahbruno rosahbruno changed the base branch from release to main September 21, 2022 12:58
@rosahbruno rosahbruno mentioned this pull request Sep 21, 2022
Merged
@rosahbruno rosahbruno marked this pull request as ready for review September 21, 2022 17:57
@auto-assign auto-assign bot requested a review from Dexterp37 September 21, 2022 17:57
@rosahbruno
Copy link
Contributor Author

@badboy
I am testing this out in the web project and had a question about the persistence of data. If I call accumulate, refresh the page, call accumulate again, then submit the ping I get two values in my histogram. Is this expected? Or once the user refreshes their page should the old data be cleared?

I know we chatted a bit about how this should work with timing distributions, so wanted to get your thoughts.

@badboy
Copy link
Member

badboy commented Sep 22, 2022

@badboy I am testing this out in the web project and had a question about the persistence of data. If I call accumulate, refresh the page, call accumulate again, then submit the ping I get two values in my histogram. Is this expected? Or once the user refreshes their page should the old data be cleared?

I know we chatted a bit about how this should work with timing distributions, so wanted to get your thoughts.

2 values is correct (if this is a ping lifetime).
accumulate here is the recording function, so we persist that.

(for timing distributions stop is the recording function. Only calling start, but never stop means we're not recording anything, that's why we "loose" data there. that's no issue for memory distributions, because we only have a single function to call)

@rosahbruno
Copy link
Contributor Author

@badboy
Ok great, thanks so much!

@rosahbruno rosahbruno mentioned this pull request Sep 22, 2022
Merged
4 tasks
@rosahbruno rosahbruno changed the title Bug 1716952 - Implement memory distribution metric Bug 1716952 - Implement the Memory Distribution Metric Sep 22, 2022
Dexterp37
Dexterp37 requested changes Oct 6, 2022
View reviewed changes
Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

Choose a reason for hiding this comment

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

This looks great overall. There's a few minor things to fix, but it's very close to be merge-able.

CHANGELOG.md Show resolved Hide resolved
glean/src/core/metrics/time_unit.ts Outdated Show resolved Hide resolved
glean/tests/integration/schema/metrics.yaml Outdated Show resolved Hide resolved
glean/tests/integration/schema/metrics.yaml Outdated Show resolved Hide resolved
glean/src/core/metrics/distributions.ts Outdated Show resolved Hide resolved
glean/src/core/metrics/distributions.ts Outdated Show resolved Hide resolved
glean/src/core/metrics/types/memory_distribution.ts Outdated Show resolved Hide resolved
rosahbruno added a commit to rosahbruno/glean.js that referenced this pull request Oct 6, 2022
@rosahbruno
Update changelog for mozilla#1514
cf03065
rosahbruno and others added 9 commits October 6, 2022 09:21
@rosahbruno
Implement Memory Distribution metric type
e973fa4 
Changes
- port over memory distribution metric
- port over memory distribution tests
- refactor existing, shared functionality between distributions
@rosahbruno
Update changelog for mozilla#1514
64a8706
@rosahbruno
Add missing configs for memory_distribution metric
f2ceff3
@rosahbruno
Update memory_distribution error type for samples larger than 1TB
d6d4192
@rosahbruno @Dexterp37
Update glean/src/core/metrics/time_unit.ts
839145f 
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
@rosahbruno @Dexterp37
Update glean/tests/integration/schema/metrics.yaml
e4f0c29 
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
@rosahbruno @Dexterp37
Update glean/tests/integration/schema/metrics.yaml
e74a592 
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
@rosahbruno @Dexterp37
Update glean/src/core/metrics/distributions.ts
c20ddad 
Co-authored-by: Alessio Placitelli <alessio.placitelli@gmail.com>
@rosahbruno
Rename negativeDuration to negativeSample to be more contextually cor…
df3b039 
…rect
@rosahbruno rosahbruno force-pushed the 1716952-memory-distribution-metric branch from d0d2d45 to a1d7412 Compare October 6, 2022 13:28
@rosahbruno rosahbruno requested a review from Dexterp37 October 6, 2022 13:37
@rosahbruno
Copy link
Contributor Author

@Dexterp37
Thanks for all the feedback! This is ready for another review.

@rosahbruno rosahbruno closed this Oct 6, 2022
@rosahbruno rosahbruno reopened this Oct 6, 2022
@rosahbruno rosahbruno force-pushed the 1716952-memory-distribution-metric branch from 46a1c58 to 335a5a8 Compare October 6, 2022 13:44
@rosahbruno rosahbruno merged commit 40dbdb5 into mozilla:main Oct 6, 2022
@rosahbruno rosahbruno deleted the 1716952-memory-distribution-metric branch October 6, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dexterp37 Dexterp37 Dexterp37 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rosahbruno @badboy @Dexterp37