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

Drop the Glean handle and move state into glean-core #664

Merged
merged 12 commits into from Jan 23, 2020
Merged

Conversation

badboy
Copy link
Member

@badboy badboy commented Jan 20, 2020

https://bugzilla.mozilla.org/show_bug.cgi?id=1602751

This is finally addressing the changes to move Glean's state into the glean-core crate, dropping all use of a "glean handle" through the FFI.

This will ultimatively allow others to use Glean through FFI without relying on our direct platform wrappers (think application-service's use across dynamic library boundaries).

This is a huge change and touches all parts of the code base, so I'd like review from a bigger set of eyes.
I tried structuring the commits to make sense on their own.
The "Remove the glean handle from *" parts are mostly automatic simple code changes, the other commits contain the actual logic changes.

This leaves the user-facing API untouched (Android/iOS consumers will keep working).

@auto-assign auto-assign bot requested a review from brizental Jan 20, 2020
@badboy badboy requested review from Dexterp37 and travis79 Jan 20, 2020
@codecov-io
Copy link

codecov-io commented Jan 20, 2020

Codecov Report

Merging #664 into master will decrease coverage by 0.37%.
The diff coverage is 75.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #664      +/-   ##
============================================
- Coverage     75.33%   74.95%   -0.38%     
  Complexity      266      266              
============================================
  Files            97       97              
  Lines          6077     6050      -27     
  Branches        729      735       +6     
============================================
- Hits           4578     4535      -43     
- Misses          963      976      +13     
- Partials        536      539       +3
Impacted Files Coverage Δ Complexity Δ
glean-core/src/histogram/linear.rs 64.47% <ø> (ø) 0 <0> (ø) ⬇️
glean-core/src/histogram/exponential.rs 67.81% <ø> (ø) 0 <0> (ø) ⬇️
...n/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt 66.66% <ø> (ø) 0 <0> (ø) ⬇️
glean-core/src/lib_unit_tests.rs 71.01% <0%> (-0.49%) 0 <0> (ø)
...la/telemetry/glean/private/StringListMetricType.kt 81.81% <100%> (+0.46%) 14 <0> (ø) ⬇️
.../mozilla/telemetry/glean/private/UuidMetricType.kt 79.48% <100%> (+1.43%) 11 <0> (ø) ⬇️
...etry/glean/private/CustomDistributionMetricType.kt 97.56% <100%> (-0.12%) 12 <0> (ø)
...mozilla/telemetry/glean/private/EventMetricType.kt 91.13% <100%> (-0.33%) 16 <0> (ø)
...etry/glean/private/MemoryDistributionMetricType.kt 93.18% <100%> (-0.44%) 13 <0> (ø)
...zilla/telemetry/glean/private/BooleanMetricType.kt 80.64% <100%> (+2.52%) 10 <0> (ø) ⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19af88c...90a4540. Read the comment docs.

Copy link
Contributor

@Dexterp37 Dexterp37 left a comment

This looks good from an high level look. I'd like to take a closer look tomorrow, but already looks solid to me.

@badboy did you test this with our instrumented tests as well?

glean-core/src/lib.rs Show resolved Hide resolved
glean-core/src/database/mod.rs Outdated Show resolved Hide resolved
glean-core/src/database/mod.rs Outdated Show resolved Hide resolved
@@ -41,8 +41,8 @@ class Glean:
>>> Glean.initialize(application_id="my-app", application_version="0.0.0", upload_enabled=True)
"""

# The handle to the underlying Rust object
_handle = 0 # type: int
# Whether Glean was initialized
Copy link
Contributor

@Dexterp37 Dexterp37 Jan 20, 2020

Choose a reason for hiding this comment

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

@mdboom could you please take a look at least at the Python changes?

@badboy
Copy link
Member Author

badboy commented Jan 20, 2020

@badboy did you test this with our instrumented tests as well?

I did forget that we don't run these for Android just yet on CI. I'll run them locally.

@brizental
Copy link
Member

brizental commented Jan 20, 2020

So, I understand the changes made here, but I don't feel confortable approving / rejecting them, because I don't have much experience with the FFI part of the code specially. I have looked throughly at the diff and don't see any problems, but I will remove myself from the reviewers list :)

One thing that is not completely clear to me is how Keep the stores open all the time (00d45ed) directly relates to "Drop the Glean handle and move state into glean-core". Would you mind explaining that a bit further?

Thanks and cool work!

@brizental brizental removed their request for review Jan 20, 2020
@badboy
Copy link
Member Author

badboy commented Jan 21, 2020

One thing that is not completely clear to me is how Keep the stores open all the time (00d45ed) directly relates to "Drop the Glean handle and move state into glean-core". Would you mind explaining that a bit further?

You are right in that this is unrelated. I will pull this out of this PR and submit it as its own (and include more of a reason "why" I did it)

mdboom
mdboom approved these changes Jan 21, 2020
Copy link
Contributor

@mdboom mdboom left a comment

LGTM. This is actually a really nice simplification (aside from it being required for the multi-language-in-the-same-runtime future of FOG).

@@ -4,29 +4,25 @@

use ffi_support::FfiStr;

use crate::{define_metric, handlemap_ext::HandleMapExtension, GLEAN};
use crate::{define_metric, handlemap_ext::HandleMapExtension, with_glean_value};
Copy link
Contributor

@mdboom mdboom Jan 21, 2020

Choose a reason for hiding this comment

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

nit: This function name didn't immediately make sense to me. Maybe with_glean_singleton?

glean-core/ffi/src/counter.rs Outdated Show resolved Hide resolved
glean-core/ffi/src/lib.rs Show resolved Hide resolved
@badboy
Copy link
Member Author

badboy commented Jan 22, 2020

BUILD SUCCESSFUL in 19s
103 actionable tasks: 4 executed, 99 up-to-date

Something is happening when I run ./gradlew connectedAndroidTest, so tests pass :)

badboy added 12 commits Jan 23, 2020
This allows to keep all global state for Glean itself in the core crate.
This removes the state from the FFI layer, allowing use of the core
crate alone in situations where we don't want to go through the FFI.
For reset we want to close the database early and then reopen it again
with the new configuration.
Mostly mechanical changes applied using `fastmod`:`

fastmod -e kt 'glean_handle: Long, ?' ''
fastmod -e kt 'Glean.handle, ?' ''
fastmod -e kt -m '^\s+\n' ''
Mostly mechanical changes applied using `fastmod`:`

fastmod -e swift 'glean_handle: Uint64, ?' ''
fastmod -e swift 'Glean.shared.handle, ?' ''
fastmod -e swift -m '^\s+\n' ''
Mostly mechanical changes applied using `fastmod`:`

fastmod -e py 'cls._handle, ?' ''
fastmod -e py 'Glean._handle, ?' ''
lint/style fix
@badboy badboy merged commit 896a2a6 into master Jan 23, 2020
@badboy badboy deleted the move-global branch Jan 23, 2020
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

6 participants