-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
refactor(firebase_performance): refactor underlying implementation for the "handler system" so the handlers are generated on native side #9334
Conversation
…example app code)
…example app code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a quick code review, everything seems fine to me.
I will start to use it, here on my dev machine, to ensure nothing is missing. Thanks for the great work!
btw, the question is, do you need someone to review it? |
@rwrz - thanks! We'll aim to get this PR merged at some point next week hopefully 🤞. In the mean time, if you do have any feedback could you update this channel, please? 😄 To your question, if you wish to leave a review, that would be awesome! The more eyes the better 💪 . However, the PR requires 2 approvals from contributors before it can be merged 👌 |
@rwrz Have you started to use this in your project? |
Yes. We were using it in DEV for a while, and now it is going to production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm wondering if we should make it a major version since it's a major rewriting of the firebase_performance plugin? It would also help version it properly and revert back to a previous version if anything would go wrong?
@Lyokone Agreed, I must've missed the ( |
# Conflicts: # packages/firebase_performance/firebase_performance/example/lib/firebase_config.dart
I'm a bit confused by the |
this._url, | ||
this._httpMethod, | ||
) : super(); | ||
|
||
final int _methodChannelHandle; | ||
final int _httpMetricHandle; | ||
late final int _httpMetricHandle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not be simplified? Instead use int? _httpMetricHandle
within the start/stop methods to detect whether it's been started/stopped already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated implementation, but you still need an additional boolean
to check if it has ran once. Otherwise, you would set _httpMetricHandle
to null
again and you would be able to start/stop again.
@Ehesp I've made it a breaking change because of the following reasons:
|
@russellwheatley I had to switch back to "master". Your implementation is lacking the possibility to create "custom traces".
|
@rwrz you must be using the old version because that method channel call ( You can also check the CI which is running these Firebase Performance e2e tests for custom traces. |
# Conflicts: # packages/firebase_performance/firebase_performance/example/.firebaserc # packages/firebase_performance/firebase_performance/example/android/app/google-services.json # packages/firebase_performance/firebase_performance/example/ios/Runner.xcodeproj/project.pbxproj # packages/firebase_performance/firebase_performance/example/ios/firebase_app_id_file.json # packages/firebase_performance/firebase_performance/example/lib/firebase_options.dart # packages/firebase_performance/firebase_performance/example/lib/main.dart
Any update on this? |
Description
Removed the handler system on the Method channel. android & iOS track the
HttpMetric
&Trace
instances on their implementations. This makes it much safer and easier to understand.Something that I wasn't sure on was behaviour for hot restart. Implementation will now
stop()
and remove allHttpMetric
&Trace
instances on a hot restart. android & iOS.I've also added additional e2e tests for
start()
andstop()
. You can nowstart()
andstop()
HttpMetric
&Trace
instances as many times as you wish (They only go to the native side on the firststart()
andstop()
. So no side effects to calling multiple times.)To clarify,
HttpMetric
&Trace
are a one time only transaction. Once stopped, they are removed on the native side.I've also committed like the following to make it a bit easier to follow:
Related Issues
fixes #7465, #7447
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?