Skip to content

add new MetaMetricsController#9857

Merged
brad-decker merged 20 commits intodevelopfrom
meta-metrics-background
Dec 2, 2020
Merged

add new MetaMetricsController#9857
brad-decker merged 20 commits intodevelopfrom
meta-metrics-background

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Nov 11, 2020

Adds new MetaMetricsController

  • Moves most constants previously declared in shared/modules/metametrics to shared/constants/metametrics
  • Moves all global type declarations to shared/constants/metametrics
  • Changes the segmentNoop into 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 without flushImmediately set to true.
  • Moves as many event/page tracking implementation details from the UI into a new MetaMetrics controller.
  • Moves and migrates metaMetrics related fields from preferences controller to MetaMetrics controller.
  • No longer required to have a factory for generating trackMetaMetricsEvent functions. As many details as possible are tracked in the new controller's store instead of having to be provided by the consumer of .trackEvent.
  • Also adds .getNetworkIdentifier method on the network controller as it didn't' really make sense to have in the Metametrics controller.
  • Adds 95%+ code coverage on the new controller, where previously the MetaMetrics implementation was untested

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [fd5b788]
Page Load Metrics (334 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint298138116
domContentLoaded2525833327636
load2545843347636
domInteractive2525823327636

@brad-decker
Copy link
Copy Markdown
Contributor Author

@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.

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

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

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [c14bfd3]
Page Load Metrics (389 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30493853
domContentLoaded24074338813967
load24274538913967
domInteractive24074238713967

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ab6269c]
Page Load Metrics (378 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29103462211
domContentLoaded25064937611153
load25265137811153
domInteractive25064937611153

@brad-decker brad-decker force-pushed the meta-metrics-background branch from ab6269c to 8995bd1 Compare November 12, 2020 18:31
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [8995bd1]
Page Load Metrics (398 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint317940105
domContentLoaded24768639713464
load24868739813464
domInteractive24668539713464

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [b959c37]
Page Load Metrics (332 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint287537105
domContentLoaded2555703308541
load2565713328541
domInteractive2555703308541

@brad-decker brad-decker changed the title wip: events from the background add new MetametricsController Nov 12, 2020
@brad-decker brad-decker force-pushed the meta-metrics-background branch from ec75c2a to 3bf12df Compare November 12, 2020 22:40
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [3bf12df]
Page Load Metrics (522 ± 107 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28111632512
domContentLoaded2441052520223107
load2491054522223107
domInteractive2441051520223107

@brad-decker brad-decker marked this pull request as ready for review November 12, 2020 23:00
@brad-decker brad-decker requested a review from a team as a code owner November 12, 2020 23:00
@brad-decker brad-decker requested review from danjm and removed request for danjm November 12, 2020 23:00
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [9777c3d]
Page Load Metrics (366 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28380517636
domContentLoaded24859036410751
load25059236610751
domInteractive24859036410751

@brad-decker brad-decker marked this pull request as draft November 13, 2020 23:38
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [af7d182]
Page Load Metrics (413 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint28453642
domContentLoaded25876541215273
load26076741315273
domInteractive25876541215273

@brad-decker brad-decker marked this pull request as ready for review November 16, 2020 18:31
@brad-decker
Copy link
Copy Markdown
Contributor Author

@Gudahtt this is ready now, I updated it to move the state from preferences controller.

@brad-decker brad-decker changed the title add new MetametricsController add new MetaMetricsController Nov 16, 2020
@brad-decker brad-decker force-pushed the meta-metrics-background branch from af7d182 to cc4a7b4 Compare November 17, 2020 19:56
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Dec 2, 2020

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [1eab45e]
Page Load Metrics (513 ± 41 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30101602311
domContentLoaded3196175118541
load3216185138541
domInteractive3196165118541

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [41624b4]
Page Load Metrics (466 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30106532412
domContentLoaded29577046413264
load29677246613264
domInteractive29577046413263

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Dec 2, 2020

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 🤔

@Gudahtt Gudahtt force-pushed the meta-metrics-background branch from 41624b4 to 4acc259 Compare December 2, 2020 20:58
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [4acc259]
Page Load Metrics (498 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3095552412
domContentLoaded3205924967938
load3215944987938
domInteractive3205924967938

Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

I have tested the migration locally, and it all seems to work correctly.

@brad-decker brad-decker merged commit 0653a48 into develop Dec 2, 2020
@brad-decker brad-decker deleted the meta-metrics-background branch December 2, 2020 21:41
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants