[firebase_performance] Java side of FirebasePerformance rewrite #1529
Conversation
There was a problem hiding this comment.
Previously we had a mechanism for registering httpMetrics and traces and unregistering them when stop() was called using the handle so that they could be garbage collected on the Java side. With this refactor, it seems like FlutterHttpMetric and FlutterTrace will leak after stop() is called. The channel has a reference to the registered FlutterHttpMetric/FlutterTrace and there's no unregister mechanism on the Java side. Perhaps unregistering/destroying the method channel in stop() would make sense?
| import io.flutter.plugin.common.MethodCall; | ||
| import io.flutter.plugin.common.MethodChannel; | ||
|
|
||
| public class FlutterFirebasePerformance implements MethodChannel.MethodCallHandler { |
There was a problem hiding this comment.
It seems like you split the plugin class into two parts which is different from what we're doing with other first-party plugins. Is this a new convention that you'd like to see applied to all of Flutterfire and plugins generally? If so we could file an issue to update the flutter create -t plugin behavior. Or if not, it might be better to leave it the way it is for consistency.
There was a problem hiding this comment.
No, I only separated this part as the FirebasePerformance plugin doesn't have any other classes that you can instantiate on there own. (e.g. to create an object you use FirebasePeformance.newTrace() or FirebasePeformance.newHttpMetric(). ML Kit works the same way, but Firebase Dynamic Links doesn't.
I created this PR to get feedback on creating separate MethodChannels for each class. Although its require more code, i think it's easier to read/maintain.
There was a problem hiding this comment.
I'm on board with splitting things into more classes, generally. I'm ok with using it for this plugin as a one-off and we can discuss offline if we want to adopt the approach for plugins generally.
|
Thats a good point. I'll add deregistration. |
There was a problem hiding this comment.
Previously if you called getAttributes and getMetric after trace.stop() it would work, but now it won't work because we're unregistered from the channel and the data only lives on the native side. And HTTPMetric is losing the getter for properties like responseCode. These seems like potentially major regressions in functionality... I'm not sure how to support these use cases with the new approach but perhaps we should be caching the data on the Dart side after all.
bparrishMines
changed the title
bparrishMines
force-pushed
the
perf_rewrite
branch
from
2682a59 to
2a3f0b2
Compare
bparrishMines
changed the title
collinjackson
changed the title
|
Closing in favor of #1557 |
…lutter#1529) * Move firebase_analytics to firebase_analytics/firebase_analytics * Update homepage for firebase_analytics * Add firebase_analytics_platform_interface package * dartfmt * Fix analysis errors * Respond to review comments
4 participants
Description
Possible reformat of FirebasePerformance plugin
Related Issues
Dart: #1530
iOS: #1556
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?