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

Bug 1790501 - Add count to DistributionData payload #2196

Merged

Conversation

rosahbruno
Copy link
Contributor

@rosahbruno rosahbruno commented Sep 19, 2022

Update DistributionData object to include count so that a dev can easily check how many samples end up in the distribution. This change effects timing, memory, and custom distributions.

Copy link
Contributor Author

@rosahbruno rosahbruno left a comment

Choose a reason for hiding this comment

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

Seeing some weirdness that I don't understand with i64 & u64 in the Kotlin tests. If anyone more knowledgeable about Rust or Kotlin has any ideas, it would be much appreciated!

@rosahbruno
Copy link
Contributor Author

@travis79
No rush on this, but do you have any ideas about the issue I described above? The Rust tests don't seem to care if I use an i64 or a u64, but Kotlin seems to not like the type conversion and throws weird errors when trying to run the tests.

@travis79
Copy link
Member

@travis79 No rush on this, but do you have any ideas about the issue I described above? The Rust tests don't seem to care if I use an i64 or a u64, but Kotlin seems to not like the type conversion and throws weird errors when trying to run the tests.

So, for the longest time unsigned types were a "prototype" feature in kotlin, so we used signed types even though we never had a use case to record negative values. So I think any of the values coming across the FFI should be signed types because of this.

What failures were you seeing on the Kotlin side, maybe that will help me to point you in the right direction to get this working with an i64

@travis79 travis79 force-pushed the 1790501-fix-timing-distribution-docs branch from 5d1d0e3 to 9ff8e14 Compare September 22, 2022 14:54
This was what I originally tried but tests were failing locally. Travis
tested and said it looked fine for him and I want to see if CI runs.
@rosahbruno
Copy link
Contributor Author

Talked with @travis79 and looks like my issues were caused (potentially, unconfirmed) by gradle caching. I ran the Kotlin tests via make test-kotlin instead of ./gradlew and everything worked as expected - the CI tests also passed!

So looks like the issue I was seeing was just a local thing, thanks @travis79 !

@rosahbruno rosahbruno marked this pull request as ready for review September 23, 2022 13:49
@rosahbruno rosahbruno requested a review from a team as a code owner September 23, 2022 13:49
@rosahbruno rosahbruno requested review from chutten and removed request for a team September 23, 2022 13:49
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

This is looking great! I had just a couple of non-blocking nits, and I don't see a need to re-review this. 🎉

docs/user/reference/metrics/custom_distribution.md Outdated Show resolved Hide resolved
glean-core/tests/timing_distribution.rs Outdated Show resolved Hide resolved
@rosahbruno rosahbruno merged commit 945a940 into mozilla:main Oct 5, 2022
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

2 participants