search

mozilla / security-advisor-shield-study

Mozilla Public License 2.0
2 stars 7 forks source link

Update Telemetry Logging #29

Closed casebenton closed 7 years ago

casebenton commented 8 years ago

@mythmon r?

casebenton commented 8 years ago

That was a great idea to unify all of the logging methods. Deleting all of the old methods was satisfying.

mythmon commented 8 years ago

This works, though I think you didn't take it far enough. You could have also unified all the events into one named event with a parameter, which help remove a lot of code as well.

casebenton commented 7 years ago

@mythmon I tried unifying the events, but I had some trouble. Many of them are disparate enough in how they operate that attempting to unify them was difficult and introduced a lot of bugs. Too be clear, you were talking about the events in notify.js, right? How do you think that I should unify them, seeing that many of the event handlers behave in different ways?

mythmon commented 7 years ago

After reading through this code again, I think that the unification of events I had in mind might not make as much sense.

I was talking about the event handler in Advisor.js. I think that having an event handler that passes a parameter would help, like this:

this.panel.port.on('uiEvent', type => {
  this.telemetryLog.logUIEvent(type, this.activeRecDomain);
});

That wouldn't work well for some of the events though. I'd be fine not making this change. Making telemetryLog already helped a lot.

mythmon commented 7 years ago

This is fine, any extra changes are just bike shedding at this point. Though there are merge conflicts.