mozilla-lockwise / lockbox-extension

Experimental Firefox extension for login management experiences, not being actively developed
Mozilla Public License 2.0
127 stars 26 forks source link

Telemetry front-end event recording is brittle #171

Open linuxwolf opened 7 years ago

linuxwolf commented 7 years ago

The current calls to recordEvent() are performed in places that works as we expect today, but might result in "false positives" in the future:

[ follow up from points made in #156 ]

jimporter commented 7 years ago

One option is to perform as much of our telemetry in our Redux reducers as possible, although that might actually be a bad practice, since they're supposed to be pure functions. It's arguable if logging actions makes something "impure" or not though...

linuxwolf commented 7 years ago

I care less about some notion of architectural purity than I do:

A Redux reducer probably works well in a lot of cases, but it's ok if there are a couple of (documented) exceptional paths because of how things really work.

jimporter commented 7 years ago

I think the main thing is that keeping reducers pure makes testing and maintenance simpler, as well as working better with devtools (so you can do stuff like time-travel to previous states). Another option might be to create Redux middleware that intercepts actions, since that's how redux-logger works. I'll do some research into this starting tomorrow and see what the right way is.

devinreams commented 6 years ago

@jimporter is this addressed with your middleware changes? or still something worth considering / doing here?

linuxwolf commented 6 years ago

This probably ought to be a separate issue, but telemetry recording can make the overall app brittle. There are a number of (still not understood) reasons for recording failure, it causes an uncaught failure in our code.

Personally, if it's a question between guaranteeing telemetry recording versus guaranteeing running, I prefer to let the telemetry fail but keep the app moving forward.