mozilla / telemetry-dashboard

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

[wrapper] make the selected bucket for enumerated histograms configurable #558

Open mozkeeler opened 6 years ago

mozkeeler commented 6 years ago

As far as I can tell, BUCKET_INDEX_FOR_ENUMERATED controls which bucket of an enumerated histogram to use. Currently it is a constant set to 0. It would be nice to be able to pick this in the passed-in params.

ab0092 commented 6 years ago

@chutten i would like to work on this, could you or @mozkeeler help me with some more details?

mozkeeler commented 6 years ago

My guess would be adding a parameter to TelemetryWrapper.go https://github.com/mozilla/telemetry-dashboard/blob/f66351757f13829807efd3c661ae28a0bcf5899a/wrapper/telemetry-wrapper.js#L9 that gets passed to evoTime https://github.com/mozilla/telemetry-dashboard/blob/f66351757f13829807efd3c661ae28a0bcf5899a/wrapper/telemetry-wrapper.js#L348 that tells it which bucket(s) to use. I'm not familiar with this project, though, so that might be totally off base.

chutten commented 6 years ago

@mozkeeler is correct. It might even be nice to support multiple bucket indices, but starting with just one is probably safest.

ab0092 commented 6 years ago

@chutten , @mozkeeler i have figured out what exactly is to be done but i am failing to figure out in which interface exactly is telemetry-wrapper.js is being used i have looked into the measurement and evolution dashboard but could not find it there either. Any help regarding this would be great.. Thanks..

chutten commented 6 years ago

Presently the most public user of telemetry-wrapper are dashboards generated by the Dashboard Generator. Choose the parameters, add to your dashboard, then click Generate to get a custom dashboard in a codepen.

Here's an example where you just need to click Generate Dashboard and it'll give you a website using the telemetry-wrapper.

To get one that's using the parts of the API that you'll be fixing I leave as an exercise for the reader :) (a hint: A11Y_CONSUMERS is enumerated, and generating a dashboard displaying an evolution of it shouldn't be too difficult)

chutten commented 6 years ago

Oh, and here's the documentation for wrapper, which may help: https://github.com/mozilla/telemetry-dashboard/tree/gh-pages/wrapper

ab0092 commented 6 years ago

Thanks @chutten for the assist. As per my understanding the index of dashboards added to the list is to be passed as params so will the params object which will be passed to the wrapper js look something like this ??

BucketIndex field added

var plots = [ { "channel": "nightly", "version": "", "metric": "A11Y_CONSUMERS", "useSubmissionDate": false, "sanitize": true, "trim": true, "compare": "", "sensibleCompare": true, "evoVersions": 0, "bucketindex": 0 }, { "channel": "nightly", "version": "", "metric": "A11Y_CONSUMERS", "useSubmissionDate": false, "sanitize": true, "trim": true, "compare": "", "sensibleCompare": true, "evoVersions": 0, "bucketindex": 1 } ];

And if there is any misunderstanding from my end. Could you please explain about the bucket and its index. Thanks..

chutten commented 6 years ago

Looks fine, though if you'll permit me to suggest a small change, prepend evo on the bucket index parameter. Other evolution-only parameters follow that convention.

Oh, and do remember to document the new parameter in the README as well.

ab0092 commented 6 years ago

@chutten is the bucket index specific to only evolution dashboards selection ? because what i have implemented so far is basically the indexes of the _dash list is set as bucket index in plot object.

chutten commented 6 years ago

The bucket index is only referenced in evoTime I think, so it should be evolution-only. When displaying a histogram, all buckets are shown, so we didn't need to specify a default :)

ab0092 commented 6 years ago

@chutten i require some assistance in understanding what feature the evo bucket index add. As far as i have gone through the code base the current default index value of 0 is being passed to the evolution historical array as an index so i am not being able to understand the context. And any idea how i can go about testing the wrapper.js from my localhost as i keep geeting an ERR_SSL_PROTOCOL_ERROR when i try to reference the wrapper js of my localhost in codepen. Thanks..

chutten commented 6 years ago

I wonder if it is because you're importing an SSL resource on a non-secure host (localhost)? I'm not 100% sure.

As for the feature, at present if you ask for an evolution of, say, A11Y_CONSUMERS over time you get a time-series line graph. From the code we see that A11Y_CONSUMERS has a different bucket for each a11y vendor: 0 is NVDA, 1 is JAWS, 2 is OLDJAWS, and so forth.

When we're displaying just a single line in the time-series evolution, which a11y vendor's bucket should we choose? Or, more generally, when we run across this kind of data (an enumerated histogram), which bucket should we choose?

The code as presently written just uses bucket 0. So when you look at an evolution of A11Y_CONSUMERS, you are seeing the change in proportion of NVDA a11y api consumers over time.

But what if I wanted to look at JAWS instead?

That's where the bucket index you're writing comes into play: we want to allow the user of telemetry-wrapper.js to specify a non-0 bucket for these evolutions. If I want to look at JAWS in A11Y_CONSUMERS, I can specify the index of 1, for instance.

Does this make sense?

ab0092 commented 6 years ago

@chutten thank you very much for the detailed explanation. Now its is completely clear as to why is the bucket_index required. So i think when evolution is chosen in the dashboard generator interface by the user then some sort of user input (be it a text area or a select option) is required for selection of bucket index for enumerated metrices like A11Y_CONSUMERS which will be then passed to evotime in wrapper js. Is my understanding correct ??

chutten commented 6 years ago

Adding UI to the Dashboard Generator would be a lovely follow-up issue to tackle, but isn't necessary for this Issue. This one's only looking at adding the capability to telemetry-wrapper.

But if it would help to do it at the same time, that's fine as well. I leave it to you to decide whether or not to do both things right now.

ab0092 commented 6 years ago

@chutten i have done the required changes for evoBucketIndex in the generator js as well as the wrapper js to accept configurable values for bucketindex. I have not implemented any user input for now but may be later on i will come up with that as well. I have put up the changes here https://ab0092.github.io/telemetry-dashboard/dashboard-generator/index.html. Please could you review the changes and confirm if anything more is to be added. Thanks.

chutten commented 6 years ago

It seems to work well! I generated a codepen from this dashboard spec and was able to toggle to different evoBucketIndex values to show me exactly what I wanted to see.

Please submit a PR and we'll get the code reviewed and up and running!

ab0092 commented 6 years ago

@chutten i have made the PR. I have also added the evoBucket usage description in the ReadMe.md so if you could review the description that would be great.