search

mozilla / addon-recommendation-shield-study

Stand-alone verison of Add-on Recommendation for Shield Study
Mozilla Public License 2.0
3 stars 7 forks source link

TelemetryLog shouldn't be a singleton #58

Closed Osmose closed 8 years ago

Osmose commented 8 years ago

The TelemetryLog module defines a class for tracking telemetry data, and then instantiates one instance of the class an exports it. If there's only ever going to be one log ever, it shouldn't be defined as a class, but instead the methods should be defined as exports of the module, and the module itself should have private members tracking the state.

Alternatively, the class should be exported and instantiated where it is used, probably by the Recommender class.

There may even be a bug here: I can't see where these metrics are populated, which means they're reset every single time the code is reloaded, such as when Firefox starts up or when the add-on is reloaded. That needs to be tested though. Don't we want these stats that are being logged to be persistent?

Osmose commented 8 years ago

I can confirm that on DeveloperEdition, the view counts for the panel popup are reset upon browser reset and do not persist. That sounds like it will screw up the analysis.

@gregglind Is this a blocker?

Osmose commented 8 years ago

FWIW I measured panel view counts by inspecting the outgoing telemetry packet via about:telemetry.

gregglind commented 8 years ago

It is not for me. They are all timestamped. It's a nice to fix, and it means the sequential doesn't increase across session

On Aug 5, 2016 5:46 PM, "Michael Kelly" notifications@github.com wrote:

I can confirm that on DeveloperEdition, the view counts for the panel popup are reset upon browser reset and do not persist. That sounds like it will screw up the analysis.

@gregglind https://github.com/gregglind Is this a blocker?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/addon-recommendation-shield-study/issues/58#issuecomment-237983419, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKAj85fNkAT0akUwBUyMQOGT3TGibI7ks5qc71QgaJpZM4JeHYX .

gregglind commented 8 years ago

As an alternative, put a 'session id' in the telemetry. I can also sort that out very easily. It is not needed, though, and not a blocker.