Conversation
Builds ready [fd5b788]
Page Load Metrics (334 ± 36 ms)
|
|
@rekmarks and @Gudahtt not ready for a full review, but could you check and see if I'm doing anything just horrific with how I've set up this new controller? Any tips, pointers, help is appreciated -- it is working in its current state but lots to cleanup, and would like to get some tests written for the controller. |
Gudahtt
left a comment
There was a problem hiding this comment.
This general approach seems good to me! It does seem appropriate for Segment to have its own controller, since there is associated state (I see that the state hasn't been moved into the controller yet, but that can be in a follow up PR).
I've left a couple of initial comments but I haven't looked closely at most of this yet
Builds ready [c14bfd3]
Page Load Metrics (389 ± 67 ms)
|
Builds ready [ab6269c]
Page Load Metrics (378 ± 53 ms)
|
ab6269c to
8995bd1
Compare
Builds ready [8995bd1]
Page Load Metrics (398 ± 64 ms)
|
Builds ready [b959c37]
Page Load Metrics (332 ± 41 ms)
|
ec75c2a to
3bf12df
Compare
Builds ready [3bf12df]
Page Load Metrics (522 ± 107 ms)
|
Builds ready [9777c3d]
Page Load Metrics (366 ± 51 ms)
|
Builds ready [af7d182]
Page Load Metrics (413 ± 73 ms)
|
|
@Gudahtt this is ready now, I updated it to move the state from preferences controller. |
af7d182 to
cc4a7b4
Compare
|
I've pushed a commit temporarily to remove a less-than-informative assertion from the failing test. It is removed in PR #9974 as well. |
Builds ready [1eab45e]
Page Load Metrics (513 ± 41 ms)
|
Builds ready [41624b4]
Page Load Metrics (466 ± 64 ms)
|
|
Welp. I got the previous commit to pass! Must be an intermittent failure - possibly pre-existing. Nothing in this PR stands out as a likely cause 🤔 |
41624b4 to
4acc259
Compare
Builds ready [4acc259]
Page Load Metrics (498 ± 38 ms)
|
Gudahtt
left a comment
There was a problem hiding this comment.
I have tested the migration locally, and it all seems to work correctly.
Adds new MetaMetricsController
shared/modules/metametricstoshared/constants/metametricsshared/constants/metametricssegmentNoopinto a factory that creates a mocked module. This new mocked module should behave closer to the way that Segment's track method actually works (including queueing). This is helpful for testing purposes and means that e2e tests should timeout if we accidentally wait for events withoutflushImmediatelyset to true.metaMetricsrelated fields from preferences controller to MetaMetrics controller.trackMetaMetricsEventfunctions. As many details as possible are tracked in the new controller's store instead of having to be provided by the consumer of.trackEvent..getNetworkIdentifiermethod on the network controller as it didn't' really make sense to have in the Metametrics controller.Rationale
As we do not have great control over performing operations before the extension UI completes exit/close events, we cannot rely on the provided segment utilities for flushing the event queue. We also have seen that even with batching disabled (setting flushAt: 1), some event loss is still occurring. By having the background process handle communication with Segment's API, we can use the batching mechanism of Segment and capture events from short-lived UI flows.