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

Adding Glean back to the iOS megazord #5009

Closed
wants to merge 3 commits into from

Conversation

travis79
Copy link
Member

This is so we can use it to record metrics in Rust on iOS

@codecov-commenter
Copy link

codecov-commenter commented Jun 22, 2022

Codecov Report

Base: 41.45% // Head: 41.28% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (c7b69c6) compared to base (3e570ba).
Patch coverage: 72.72% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5009      +/-   ##
==========================================
- Coverage   41.45%   41.28%   -0.18%     
==========================================
  Files         167      168       +1     
  Lines       12620    12628       +8     
==========================================
- Hits         5232     5213      -19     
- Misses       7388     7415      +27     
Impacted Files Coverage Δ
components/nimbus/build.rs 100.00% <ø> (ø)
components/nimbus/src/dbcache.rs 67.30% <0.00%> (-5.61%) ⬇️
components/nimbus/src/lib.rs 64.07% <40.00%> (-0.30%) ⬇️
components/nimbus/src/enrollment.rs 94.90% <100.00%> (+0.02%) ⬆️
components/nimbus/tests/test_telemetry.rs 100.00% <100.00%> (ø)
...ponents/external/glean/glean-core/build/src/lib.rs 0.00% <0.00%> (-92.00%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@travis79 travis79 force-pushed the nimbus-oxidation branch 2 times, most recently from 3dcf93a to 07fac9d Compare June 22, 2022 19:15
@travis79
Copy link
Member Author

travis79 commented Jun 22, 2022

There's one more thing to do for this, and that's add test coverage for the metric collection in Rust, I ran into some issues with that and need to speak with Jan-Erik to see what's going on.

It also seems to fail some checks due to the import! macro in glean_metrics.rs 🤔

@travis79 travis79 force-pushed the nimbus-oxidation branch 7 times, most recently from cac7e84 to 2ebfcf1 Compare June 23, 2022 18:43
@travis79 travis79 requested review from jhugman and tarikeshaq June 23, 2022 19:05
travis79 added a commit to travis79/rust-components-swift that referenced this pull request Jun 23, 2022
This needs to land in concert with mozilla/application-services#5009 and downstream PRs in Focus iOS and Firefox iOS
@travis79
Copy link
Member Author

Seems I forgot to move the exposure event to Rust completely for iOS... not quite ready yet.

Copy link
Contributor

@jhugman jhugman 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 lovely. 😻

I have added comments which are mostly filing bugs and questions for my own understanding.

I haven't reviewed anything in xcodeproj files, or .h files, and so we should probably ask one of the iOS teams to look at those and give a r+.

From my point of view, this is brilliant, it's been a long time coming, so thank you.

components/nimbus/ios/Nimbus/Nimbus.swift Show resolved Hide resolved
components/nimbus/ios/Nimbus/Nimbus.swift Show resolved Hide resolved
components/nimbus/src/enrollment.rs Show resolved Hide resolved
components/nimbus/ios/Nimbus/GleanPlumbHelpers.swift Outdated Show resolved Hide resolved
components/nimbus/ios/Nimbus/Nimbus.swift Outdated Show resolved Hide resolved
components/nimbus/src/lib.rs Show resolved Hide resolved
components/nimbus/src/nimbus.udl Show resolved Hide resolved
components/nimbus/tests/test_telemetry.rs Show resolved Hide resolved
Copy link
Contributor

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Thanks a ton @travis79 🚀 🎉 !! Once we get this through we should have a good story for iOS

I had a couple of blocking thoughts/comments that we might need to work out before we land this:

  1. The comment about impact on android and possibly feature gating
  2. The uniffi version compatibility comment

The rest are nits and questions. I'll timebox sometime to think about the uniffi comment and see if we can comeup with a good way to move forward!

.circleci/config.yml Show resolved Hide resolved
components/nimbus/Cargo.toml Outdated Show resolved Hide resolved
components/nimbus/Cargo.toml Outdated Show resolved Hide resolved
components/nimbus/build.rs Outdated Show resolved Hide resolved
components/nimbus/build.rs Outdated Show resolved Hide resolved
components/nimbus/ios/Nimbus/GleanPlumbHelpers.swift Outdated Show resolved Hide resolved
components/nimbus/src/enrollment.rs Outdated Show resolved Hide resolved
components/nimbus/src/lib.rs Show resolved Hide resolved
megazords/ios-rust/Cargo.toml Outdated Show resolved Hide resolved
megazords/ios-rust/build-xcframework.sh Show resolved Hide resolved
@travis79 travis79 force-pushed the nimbus-oxidation branch 5 times, most recently from 09acbaa to f8364da Compare August 2, 2022 12:55
@travis79 travis79 requested a review from tarikeshaq August 8, 2022 13:44
@travis79 travis79 force-pushed the nimbus-oxidation branch 4 times, most recently from e084833 to 5d90897 Compare August 9, 2022 13:21
travis79 added a commit to travis79/rust-components-swift that referenced this pull request Sep 20, 2022
This needs to land in concert with mozilla/application-services#5009 and downstream PRs in Focus iOS and Firefox iOS Strip Nimbus Utils directory in generate.sh
@travis79 travis79 force-pushed the nimbus-oxidation branch 11 times, most recently from c7eff54 to 352ca08 Compare September 23, 2022 20:48
@travis79 travis79 force-pushed the nimbus-oxidation branch 4 times, most recently from 26fc521 to 87c23b4 Compare October 12, 2022 12:07
This includes conditional compilation for this feature only on iOS, Android will still need to record things in Kotlin for the time being.
travis79 added a commit to travis79/rust-components-swift that referenced this pull request Oct 24, 2022
This needs to land in concert with mozilla/application-services#5009 and downstream PRs in Focus iOS and Firefox iOS Strip Nimbus Utils directory in generate.sh
@tarikeshaq
Copy link
Contributor

@travis79 since this has gotten a little stale, do you know what the priority of this work is now? (i.e do we expect we'll get to it anytime soon?)

I don't object to keeping the PR open until we get to it, just curious what a timeline might look like if we have one?

@travis79
Copy link
Member Author

@tarikeshaq Yes, the priorities on this shifted a bit. This is still desirable to do but I don't have time to get back to it immediately. Ultimately I'd love to be able to solve both iOS and Android when we get back to this. Let's close this in favor of starting fresh without any bitrot to deal with.

@travis79 travis79 closed this Feb 23, 2023
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.

4 participants