search

mozilla / telemetry-dashboard

Web-frontend for aggregated telemetry data
telemetry.mozilla.org
Other
89 stars 113 forks source link

Fix#526: Probe dictionary stats should only count "release" probes for release channel #532

Closed Insiyaa closed 6 years ago

Insiyaa commented 6 years ago

Oh I missed that! Made the required changes! Here's the link. Please review!

Insiyaa commented 6 years ago

The changes are live. Please review:)

Sylvia23 commented 6 years ago

@Insiyaa One thing that I noticed. There are two legends here when the channel is set to release where optin has no significance. I think that shouldn't be there. Does that sound right? Thanks!

Insiyaa commented 6 years ago

@Sylvia23 Yeah I though of it too. But I'm not sure about implementing this. Like I think I've to set its display to None on some condition, but I'm not able to figure that out. It'd be great if you could hint me the direction:)

Sylvia23 commented 6 years ago

@Insiyaa I think you can try someelement.style.display = none; method of styling inside JS or legend.remove() for condition when channel is set to release for optin legend in the explore.js itself. I can give you a more clearer view after doing some queries on the variables. As of now, I think above should work. I will check and tell you ASAP. Thanks!

Sylvia23 commented 6 years ago

Hi @Insiyaa, I was able to solve the issue after understanding the codebase of explore.js specially the functions getMeasurementCountsPerVersion() and renderProbeStats(). We can easily solve the issue by just counting the optin and optout values here by declaring them globally so that we can use the values in the function renderProbeStats() as well. After doing above steps, an if-else condition needs to be put just before the legend is declared here, to generate legends for only non-zero values. Put the below code-snippet here.

if(count_optin == 0) columns = ["optout"]; else if (count_optout == 0) columns = ["optin"];

This way, legend is only generated for non-zero values of the columns(data).

I have also tested it for all the channels like beta, release etc. on my local machine, it works fine for me. This method solves the legend issue generally, not only for channel release. I hope this helps. Thanks!

Insiyaa commented 6 years ago

Thanks a lot @Sylvia23 for taking the time out to guide me!:) Your explanation made it very clear... I'll make the required changes soon!

Sylvia23 commented 6 years ago

@Insiyaa Cheers !

Insiyaa commented 6 years ago

Made the changes! Thanks @Sylvia23! @georgf please review here. :smile:

Insiyaa commented 6 years ago

Thanks a lot @georgf for guiding me so patiently! Please review and let me know if any other changes are required...

Insiyaa commented 6 years ago

The case new_in was incomplete. Please review the fix. Changes are live here.