consolidate segment setup by brad-decker · Pull Request #9617 · MetaMask/metamask-extension

brad-decker · 2020-10-15T21:15:09Z

in #9509 a good step was made to move segment setup up from one of the controllers in the background process. This goes a step further by deduplicating a big chunk of setup between the frontend and background.

Bug fix, categories aren't attached correctly for background events

Improvements

adds environment_type to background events
adds chain_id to all events
updates network to either be the rpcUrl, if provider.type is rpc, or just provider.type otherwise.

metamaskbot · 2020-10-15T21:38:05Z

Builds ready [956a6df]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (402 ± 43 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	33	103	49	21	10
		domContentLoaded	295	618	399	89	43
		load	297	619	402	89	43
		domInteractive	295	617	399	89	43

metamaskbot · 2020-10-16T19:51:45Z

Builds ready [e1b0552]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (411 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	32	89	42	15	7
		domContentLoaded	247	634	409	112	54
		load	249	635	411	112	54
		domInteractive	247	633	408	112	54

rekmarks

I'm a big fan of this change!

My comments are mostly nits and questions, but there's on thing re: the metaMetricsSendCount that we need to address in this or (I think I'd prefer) a later PR.

app/scripts/lib/rpc-method-middleware/handlers/log-web3-usage.js

app/scripts/metamask-controller.js

shared/modules/metametrics.js

ui/app/selectors/selectors.js

rekmarks · 2020-10-19T16:21:11Z

shared/modules/metametrics.js

+    // to be updated to work with the new tracking plan. I think we should use
+    // a config setting for this instead of trying to match the event name
+    const isSendFlow = Boolean(event.match(/^send|^confirm/u))
+    if (isSendFlow && metaMetricsSendCount && !sendCountIsTrackable(metaMetricsSendCount + 1)) {


Looking at the ConfirmTransactionBase component, aren't we racing to increment the metaMetricsSendCount?

This may be a preexisting problem, but we should increment and update metaMetricsSendCount in a single place, ideally in some MetaMetrics handler function.

It's OK if we do that in a separate PR, but we should absolutely do it.

I'll address this in a separate PR

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

metamaskbot · 2020-10-19T17:14:20Z

Builds ready [c04720e]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (484 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	82	41	11	5
		domContentLoaded	303	771	482	137	66
		load	308	772	484	137	66
		domInteractive	302	771	482	137	66

brad-decker · 2020-10-19T20:47:33Z

@rekmarks I addressed your feedback in the latest commit

metamaskbot · 2020-10-19T21:08:36Z

Builds ready [d1bbc20]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (390 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	78	39	12	6
		domContentLoaded	300	629	388	91	44
		load	302	631	390	91	44
		domInteractive	300	629	388	91	44

app/scripts/metamask-controller.js

ui/app/selectors/selectors.js

brad-decker · 2020-10-19T22:13:47Z

made those requested changes @rekmarks! thanks for the feedback.

metamaskbot · 2020-10-19T22:32:26Z

Builds ready [caa5ee0]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (407 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	40	34	3	2
		domContentLoaded	293	676	405	124	60
		load	295	677	407	125	60
		domInteractive	293	675	405	124	60

rekmarks

LGTM!

brad-decker · 2020-10-20T15:40:30Z

Waiting for a review from @Gudahtt before I merge as he expressed interest in doing so; I want to get additional buy-in on the approach as it's the first shared module. Thanks for your reviews, @rekmarks. I've already incorporated these changes into my WIP branches off of this one to great effect.

metamaskbot · 2020-10-20T18:11:47Z

Builds ready [b47fbb3]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (445 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	77	38	10	5
		domContentLoaded	298	753	443	137	66
		load	299	755	445	137	66
		domInteractive	297	753	443	137	66

app/scripts/metamask-controller.js

shared/modules/metametrics.js

metamaskbot · 2020-10-21T18:22:11Z

Builds ready [1fd82aa]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (404 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	78	38	10	5
		domContentLoaded	295	700	402	123	59
		load	296	701	404	123	59
		domInteractive	295	699	402	123	59

Gudahtt

LGTM!

brad-decker requested review from Gudahtt and rekmarks October 15, 2020 21:15

brad-decker marked this pull request as ready for review October 15, 2020 21:45

brad-decker requested a review from a team as a code owner October 15, 2020 21:45

consolidate segment setup

e1b0552

brad-decker force-pushed the share-segment-implementation branch from 956a6df to e1b0552 Compare October 16, 2020 19:08

rekmarks requested changes Oct 19, 2020

View reviewed changes

Update ui/app/selectors/selectors.js

c04720e

Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>

addressing changes

d1bbc20

rekmarks reviewed Oct 19, 2020

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

rekmarks reviewed Oct 19, 2020

View reviewed changes

ui/app/selectors/selectors.js Outdated Show resolved Hide resolved

brad-decker added 2 commits October 19, 2020 17:09

addl changes

df61cae

add comment with reasoning for omitting

caa5ee0

rekmarks previously approved these changes Oct 19, 2020

View reviewed changes

brad-decker added the DO-NOT-MERGE Pull requests that should not be merged label Oct 20, 2020

construct new Promise correctly

b47fbb3

brad-decker dismissed rekmarks’s stale review via b47fbb3 October 20, 2020 17:52

Gudahtt reviewed Oct 21, 2020

View reviewed changes

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved

shared/modules/metametrics.js Outdated Show resolved Hide resolved

shared/modules/metametrics.js Outdated Show resolved Hide resolved

Gudahtt reviewed Oct 21, 2020

View reviewed changes

shared/modules/metametrics.js Outdated Show resolved Hide resolved

brad-decker added 2 commits October 21, 2020 12:48

feedback addressed

fb202ec

remove defaultParam param

1fd82aa

brad-decker removed the DO-NOT-MERGE Pull requests that should not be merged label Oct 21, 2020

Gudahtt approved these changes Oct 21, 2020

View reviewed changes

brad-decker merged commit e5688c0 into develop Oct 21, 2020

brad-decker deleted the share-segment-implementation branch October 21, 2020 21:10

github-actions bot locked and limited conversation to collaborators Oct 21, 2020

Conversation

brad-decker commented Oct 15, 2020

Improvements

Uh oh!

metamaskbot commented Oct 15, 2020

Uh oh!

metamaskbot commented Oct 16, 2020

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rekmarks Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

brad-decker Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Oct 19, 2020

Uh oh!

brad-decker commented Oct 19, 2020

Uh oh!

metamaskbot commented Oct 19, 2020

Uh oh!

Uh oh!

Uh oh!

brad-decker commented Oct 19, 2020

Uh oh!

metamaskbot commented Oct 19, 2020

Uh oh!

rekmarks left a comment

Choose a reason for hiding this comment

Uh oh!

brad-decker commented Oct 20, 2020

Uh oh!

metamaskbot commented Oct 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

metamaskbot commented Oct 21, 2020

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants