Closed Bug 840097 Opened 7 years ago Closed 6 years ago

about:telemetry should have a way to copy a text histogram to the clipboard

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: ted, Assigned: avih, NeedInfo)

Details

(Whiteboard: [mentor=vladan][lang=js][good first bug][good first verify])

Attachments

(1 file, 5 obsolete files)

about:memory has the nice feature that you can copy/paste it as text. I think it uses Unicode line drawing characters. It would be cool if you could copy an individual histogram out of about:telemetry as text so you could paste it in a bug or something like that.
I don't have time to add this feature right now, but this might be a good first bug for someone with some JS experience.

The completed solution should be a button in the "Histograms" sub-section which stores an ASCII representation of the about:telemetry histograms in the user clipboard. The representation could either be a two column table mapping buckets to bucket counts or ASCII-art.

For example:

GC_MS
------------------
34-50:  1 sample
50-57:  3 samples
57-68:  2 samples
68-81:  1 sample

GC_MS
        3
       ###  2
    1  ### ###  1
___###_###_###_###___
    34  50  57  68

* We might want to flip the histogram to better deal with tall bars

To get started:

1. Read about the Telemetry histogram documentation page
https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe
2. Familiarize yourself with about:telemetry code and the Histogram_render method in particular
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js 
3. Ping me on IRC :) I'm "vladan" on the #perf channel of irc.mozilla.org
Whiteboard: [mentor=vladan][lang=js][good first bug]
After some IRC discussion, we decided on a few details:

1. Mousing over a histogram should pop up a "Copy" link within the histogram. The "Copy" link could be next to the histogram name. Some extra padding should be added to the histogram during the initial render of the histogram section in order to allow the "Copy" link to pop in without causing a reflow of the histogram.

2. Clicking on "Copy" should store a text representation of that histogram in the user's clipboard. The details of the representation are left to the implementer. This is one idea below.

GC_MS
15 samples, mean = 56, sum = 730
--------------------------------------
34-50:  ##### 2
50-57:  #################### 8
57-68:  ########## 4
68-81:  ### 1

3. In this model, the height of the tallest bar is always 20 units. It represents the bucket with the most samples. The heights of the other bars (other buckets) depend on their sample counts, e.g. if a bucket has half the samples of the biggest bucket, its height will be half the maximum (10 units)
Picture of how the |Copy| link look.
Attachment #728602 - Flags: feedback?(vdjeric)
Things are working in this patch. Some improvements are still required.
Here is a sample of the copied histogram:

WORD_CACHE_HITS_CHROME
383 samples, average = 4.5, sum = 1709
-----------------------------------
0:   0
1:  #################### 99
2:  ## 11
3:  ########### 54
4:  ########### 56
5:  ###### 30
6:  ####### 35
7:  ######### 44
8:  ## 11
9:  ###### 32
11:  # 6
15:  # 5
18:   0 

* Single and double digits at the beginning of every line is causing problem.

* After implementing |mouseover| & |mouseout| on |histogram| class, |Copy| becomes visible when the mouse is over |histogram|(div) but when it goes over other div which are inside |histogram|(div) it again goes hidden. |Copy| is itself in a separate div, pointing on which causes |mouseout| from |histogram|(div) and copy hides itself. Please suggest solution for this.

* In the code level, I tried using |getHistogramById| but it wasn't helpful in getting the proper required data or I wasn't able to figure it out. In the current code, I have used some dirty tricks(I feel it's dirty) which works fine. If the current way of implementation is fine, please suggest on improving the code readability and proper naming of the variables used.
Attachment #728603 - Flags: feedback?(vdjeric)
Looks promising :) I've been away on vacation, I'll give feedback on this patch today or early tomorrow
Comment on attachment 728603 [details] [diff] [review]
Added copy link to copy respective histogram.

Thank you for the patch :) See my comments below.

- Can we try making the "Copy" link gray or default color, but italicized? IMHO the bolded green is a bit strong, but that's just my personal taste, not a requirement :)
- In the sample histogram you provided, the alignment seems to be off for the 0 bucket. Also, as you pointed out, alignment is off for buckets which have a different number of digits in the label. We should use leading spaces for labels with fewer than max digits
- I think you want the "mouseenter"/"mouseleave" events instead of "mouseover"/"mouseout"

>+.copyText {
>+  padding: 5px;
>+  color: green;
>+  font-weight: bold;
>+}

To remain consistent with the naming style, .copyText should be .copy-text

>+const nsSupportsString = Components.Constructor("@mozilla.org/supports-string;1", "nsISupportsString");
>+function SupportsString(str) {
>+  var res = nsSupportsString();
>+  res.data = str;
>+  return res;    
>+}

Rename nsSupportsString -> SupportString and just use it in the copy function, you don't really need the SupportsString function since this code will only be called from one place.

>+function Transferable(source) {
>+  var res = nsTransferable();
>+  if ('init' in res) {
>+    if (source instanceof Ci.nsIDOMWindow)
>+      source = source.QueryInterface(Ci.nsIInterfaceRequestor)
>+                     .getInterface(Ci.nsIWebNavigation);
>+
>+    res.init(source);
>+  }
>+  return res;
>+}

Why does Transferable take a parameter if you never call it with an argument? Take a look at aboutSupport.js for another way to do this

>+    let copyHistogram = document.createElement("a");

Nitpick: rename copyHistogram to copyLink

>+    copyHistogram.className = "copyText";
>+    copyHistogram.innerHTML = "Copy";
>+    copyHistogram.id = "COPY" + aName;

Nit: Since the histogram IDs on this page are currently uppercase+underscores, change the prefix from "COPY" to "COPY_"

>+    copyHistogram.style.visibility = "hidden";

We have a "hidden" CSS class defined: copyHistogram.classList.add("hidden");

>+    console.log("loaded");

remove the logging from the final patch

>     let divStats = document.createElement("div");
>+    divStats.className = "stats";

Where is this class name used/defined?

>+  copyVisible: function copyVisible(e) {
>+    console.log("visible triggered");
>+    let id = "COPY" + String(e.target.id);
>+    console.log(id);
>+    let elem = document.getElementById(id);
>+    elem.style.visibility = "visible";
>+  },

This works but you can do it an alternate way:

showCopyLink: function Histogram_showCopyLink(aEvent) {
  let copyLink = aEvent.target.getElementsByClassName("copy-text");
  copyLink[0].classList.remove("hidden");
}

>+  copy: function copy(e) {

Naming: "copy: function Histogram_copy(aEvent) {"

>+    var str = "";

Use "let" instead and declare variables when they are first assigned

>+    let elem = document.getElementById(histId);
>+    let bars = elem.getElementsByClassName('bar');
>+    let stats = elem.childNodes[1].textContent;

It looks like you're reverse engineering the histogram data by reading the data out of the label elements in the page. This will work and it has the nice side-effect that it copies the data in the histogram when the page is rendered, as opposed to the latest data in the histograms. However, it feels a bit hacky to me and this function will need to be updated when the histogram rendering code is changed.

- I would suggest you simply use histId to get the histogram data: hgram = Telemetry.histogramSnapshots[histId];
- You can get the max bucket count with: let maxBucket = Math.max.apply(Math, hgram.counts);
- Then you can just loop from 0 to hgram.ranges.length - 1, while reading hgram.ranges[i] for the bucket label and hgram.counts[i] for bucket counts. Skip over any buckets with 0 items. In that same loop, you can output the ASCII bars. You can either use the for-loop you have now to build the "#" ASCII bars, or you can use Array(barHeight + 1).join("#")
- The histogram sum is in hgram.sum. You can get the sample count while in the loop

>+    var trans = Transferable();

Use let instead of var
Attachment #728603 - Flags: feedback?(vdjeric)
Attachment #728602 - Flags: feedback?(vdjeric)
Attached patch Copy Link added to Histograms (obsolete) — Splinter Review
Sorry for such a long delay.

This patch contains:

* Change in color of |Copy| link. It's gray now and in italic.
* Refactored the code from previous patch and used |histogramSnapshots| to get data.
* Moved |SupportString| and all copy to clipboard code inside |copy| function.
* Added leading whitespace in front of labels.

Here is how it looks now:

STARTUP_WORD_CACHE_MISSES_CHROME
19 samples, average = 5.5, sum = 105
-------------------------------------------
0    : 0
1    : ###1
2    : ###1
3    : #######2
4    : ###################5
5    : #######2
6    : #######2
7    : ###########3
9    : #######2
15   : ###1
18   : 0

Here are the things which I failed to implement:

1. Use |classList| to hide and show the copyLink. I tried |element.classList.add("hidden")| and |element.classList.remove("hidden")| but it didn't seem to behave properly. On |mouseenter|, |Copy| won't be visible. 

2. |x samples, average = xx, sum = xx| are inserted using some variables like |hgramSamplesCaption| in |Histogram|. When I did |this.hgramSamplesCaption| inside |copy| function, they were undefined. Perhaps some scope problem which I was not able to figure out. So I hardcoded that line with variables in between.

Please review it and leave comments for further improvements.
Attachment #728602 - Attachment is obsolete: true
Attachment #728603 - Attachment is obsolete: true
Attachment #736852 - Flags: feedback?(vdjeric)
Comment on attachment 736852 [details] [diff] [review]
Copy Link added to Histograms

- Looks like you're almost done :)
- The copied text should use CR+LF newlines on Windows. Use "#ifdef XP_WIN"
- Put a space between the ASCII bar and the bar's label
- I tried out the patch on my machine. What do you think of making the "Copy" link into an anchor (without CSS, except padding)? Sorry for bikeshedding ;)

>+    let copyLink = document.createElement("a");
>+    copyLink.className = "copy-text";
>+    copyLink.innerHTML = "Copy";

You shouldn't hard-code user-visible English strings in the code, they need to be localized
Put the string here: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/aboutTelemetry.properties

Nit: Use createTextNode instead of setting innerHTML:

copyLink.appendChild(document.createTextNode(this.hgramCopyCaption));

>+  showCopyLink: function Histogram_showCopyLink(aEvent) {
>+    let id = "COPY_" + String(aEvent.target.id);
>+    let elem = document.getElementById(id);
>+    elem.style.visibility = "visible";
>+  },

- You don't need the String() constructor
- Document the method with a comment, including parameters (see other functions)
- Use elem.classList.remove("hidden"). I think it didn't work for you because you hard-coded elem.style.visibility = "hidden" in Histogram.render()

>+  /**
>+   * Copy text histogram to clipboard.
>+   */
>+  copy: function Histogram_copy(aEvent) {
>+
>+    let histId = aEvent.target.id.slice(5);
>+    let str = "";

Only declare str when it's first needed. You might want to use a more descriptive name too, e.g. clipboardData

>+    
>+    // Get the required data.
>+    let sample_count = hgram.counts.reduceRight(function (a, b) a + b);

Nit: Why not just "reduce"?

>+    let average = Math.round(hgram.sum * 10 / sample_count) / 10;

let average = hgram.sum.toFixed(1);

>+    let first = true;
>+    let last = 0;

You can also use values.length == 0 instead of "first", and you can just use the final value of "i" instead of last

>+    let values = [];
>+    let max_count = 0;
>+    for (let i=0; i < buckets.length-1; i++) {

Nit: spaces around "=" and "-"

>+      if (count > max_count)
>+        max_count = count;

Nits: Use curlies. Also, you can figure out the max outside the loop using "reduce" or Math.max.apply

>+    // Form the text histogram string.
>+    let stats = sample_count + " samples, average = " + average +
>+                ", sum = " + hgram.sum;

Use Histogram.hgramSamplesCaption, etc

>+
>+    str += histId + "\n" + stats + "\n-------------------------------------------";
>+

Nit: You can use Array(N).join("-") to construct the dashes

>+    for (let [label, value] of values) {
>+      let barLength = Math.round(20 * (value / max_count));

Don't hard-code "magic numbers", declare the 20 in a const variable at the top of the file
You can merge the two for-loops if you get the max bucket count using one of the methods I suggested

>+      let labelLength = String(label).length;

No need for String()

>+    let st = Cc["@mozilla.org/supports-string;1"].createInstance(Ci.nsISupportsString);
>+    st.data = str;

Move the system clipboard manipulation into a separate function

>+    function getLoadContext() {
>+      return window.QueryInterface(Ci.nsIInterfaceRequestor)
>+                   .getInterface(Ci.nsIWebNavigation)
>+                   .QueryInterface(Ci.nsILoadContext);
>+    }

Why does this need to be a function?
Attachment #736852 - Flags: feedback?(vdjeric)
Attached patch Copy Link added to Histograms (obsolete) — Splinter Review
Here is a list of the changes:
* Added a space between the ASCII bar and it's label.
* Removed hard-coded elements and moved them to localization file.
* Used createTextNode instead of innerHTML.
* Removed String() constructor wherever not required.
* Documented the methods. Hope it's good :)
* elem.classList.add/remove works now.
* Refactored the 2 for-loops into 1. After a lot of tweaking, it seems to work correctly now. While doing so, created another function |copyToClipboardData| to reduce the code. Please see if it's good enough or some more can be done.
* Magic Numbers made as constants.

Here are what I haven't done yet and need your help:
* Using CR+LF newlines on Windows. I found that I have to use "\r\n" with #ifdef XP_WIN, but I couldn't figure out the right way of implementing it. Would be great if you could give small code samples.
* Making the "Copy" link into an anchor. It's already a link with <a>. Please add some more details.
Attachment #736852 - Attachment is obsolete: true
Attachment #740274 - Flags: feedback?(vdjeric)
Comment on attachment 740274 [details] [diff] [review]
Copy Link added to Histograms

- You need to format the histogram label area so that displaying the "Copy" element doesn't resize the histogram. I don't know how to do this with CSS off the top of my head but I'm sure you can find a solution on web dev sites. Maybe ping ttaubert on IRC?
- According to bug 432698 and debug prints triggered by the patch, it turns out the use of "mouseenter"/"mouseleave" in chrome code is discouraged for performance reasons :( We should use mouseover/mouseout instead. Sorry for leading you astray, I wasn't aware of this performance issue
- You can make the anchor element into a proper blue link by setting its href to "javascript:"
- You don't need to use String() anywhere, JS will cast numbers to strings automatically as needed
- Don't leave trailing spaces at the end of lines in aboutTelemetry.js

>+const MAX_DIGITS = 8;

This doesn't need to be hardcoded, it looks odd to have 7 spaces of padding when all the histogram labels are one digits. Just use the length of the last bucket label in the histogram (i.e. the longest label) to figure out the maximum number of digits.

>   /**
>+   * Show Copy-Histogram link
>+   *
>+   * @param aEvent node which calls this method
>+   */

aEvent is not a node, it's the object representing the event that triggered this method

>+    // Get the required data.
>+    let sample_count = hgram.counts.reduce(function (a, b) a + b);

Use camel-case for variable names (i.e. sampleCount)

>+    let average = hgram.sum.toFixed(1);

You need to divide the sample sum by the sample count to get the average

>+
>+    var clipboardData = histId + "\n" + stats + "\n" + Array(42).join("-");

Use let instead of var.
Also, Declare a NEWLINE const at the top of the file.
For example, see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkHTMLUtils.jsm#85

>+    // Render ASCII bars and append to clipboardData
>+    function copyToClipboardData(label, count, labelLength, barLength) {
>+      clipboardData += "\n" + label + Array(labelLength).join(" ") + ":";
>+      clipboardData += Array(barLength + 1).join("#");
>+      clipboardData += " " + count;
>+    }

This function can be expanded to do more of the work that is currently being done by its callers.
It should only take two inputs: the label and the count. It should figure out the label's length and bar length on its own.
Rename it to something more precise, e.g. "appendAsciiBar".

>+    let first = true;
>+    let last = 0;
>+    let label = null;
>+    let count = null;
>+    let maxBucket = Math.max.apply(Math, hgram.counts);
>+    let barLength = null;
>+    let labelLength = null;
>+    for (let i = 0; i < buckets.length; i++) {
>+      count = hgram.counts[i];
>+      label = String(buckets[i]);
>+      if (!count)
>+        continue;
>+      if (first) {
>+        first = false;
>+        if (i) {
>+          let newlabel = String(buckets[i-1]);
>+          let newcount = 0;
>+          barLength = Math.round(MAX_COPY_BAR_HEIGHT * (newcount / maxBucket));
>+          labelLength = MAX_DIGITS - (newlabel.length - 1);
>+          copyToClipboardData(newlabel, newcount, labelLength, barLength);
>+        }
>+      }
>+      last = i + 1;
>+      barLength = Math.round(MAX_COPY_BAR_HEIGHT * (count / maxBucket));
>+      labelLength = MAX_DIGITS - (label.length - 1);
>+      copyToClipboardData(label, count, labelLength, barLength);
>+    }
>+    if (last && last < buckets.length) {
>+      label = String(buckets[last]);
>+      count = hgram.counts[last];
>+      barLength = Math.round(MAX_COPY_BAR_HEIGHT * (count / maxBucket));
>+      labelLength = MAX_DIGITS - (label.length - 1);
>+      copyToClipboardData(label, count, labelLength, barLength);
>+    }

You've got the right idea, but you can make this code simpler:

let first = true;
for (let i = 0; i < buckets.length; ++i) {
  // Skip over empty buckets
  if (!hgram.counts[i]) {
    continue;
  }
  // The first bucket shown in the histogram should be an empty bucket
  if (first && i != 0) {
    appendAsciiBar(buckets[i - 1], 0);
    first = false;
  }
  appendAsciiBar(buckets[i], hgram.counts[i]);
}

// Add a trailing empty bucket
if (i < buckets.length) {
  appendAsciiBar(buckets[i], 0);
}
Attachment #740274 - Flags: feedback?(vdjeric)
^^ Sorry, you will need the "last" variable as well, same as in Histogram_unpack
Is anyone actually still working on this?

I saw someone had a bit of code that was mainly undergoing review; if you don't mind and they don't either I wrote up a quick patch in about an hour that I'd like to take a stab at your reviewing it.

If not, no harm done, not much work was wasted and I learned a bit more about the FF architecture!
(In reply to smaudet from comment #12)
> Is anyone actually still working on this?
> 
> I saw someone had a bit of code that was mainly undergoing review; if you
> don't mind and they don't either I wrote up a quick patch in about an hour
> that I'd like to take a stab at your reviewing it.
> 
> If not, no harm done, not much work was wasted and I learned a bit more
> about the FF architecture!

Hi smaudet,

I probably won't be able to be back on it for atleast 2 more weeks. Would be great if you could complete this :)

Thanks
Sounds good!

I added a vertical implementation for fun; hopefully I can contribute something extra in the end! :)
This is fine by me. You can take a look at the current patch for ideas
smaudet are you working on this? How is going on?
Flags: needinfo?(smaudet)
Uses a "Copy" button which auto-shows on hover (css only) - bottom right (left on RTL).
TODO: use localized "Copy" string.

The output looks like this:

HTTP_SUB_DNS_ISSUE_TIME
362 samples, average = 191.1, sum = 69162

  4 | 0  0%
  5 | 1  0%
 11 | 1  0%
 20 | 1  0%
 29 |** 5  1%
 35 |* 3  1%
 43 | 1  0%
 52 |** 5  1%
 63 |************ 33  9%
 77 |*********** 29  8%
 94 |****************** 50  14%
115 |************************* 69  19%
140 |********************** 61  17%
171 |**************** 43  12%
209 |******* 18  5%
564 |********** 27  7%
688 |***** 15  4%
839 | 0  0%
Attachment #740274 - Attachment is obsolete: true
Attachment #8339015 - Flags: review?(vdjeric)
Comment on attachment 8339015 [details] [diff] [review]
Add clipboard copy for about:telemetry histograms

Review of attachment 8339015 [details] [diff] [review]:
-----------------------------------------------------------------

- Use "\r\n" for linebreaks on Windows. See my earlier reviews for how
- Follow the file's style for function comments + prefix arg variable names with "a" :)
- Use localized strings (as you already pointed out)

::: toolkit/content/aboutTelemetry.js
@@ +85,5 @@
> +  for (let i = 0; i < num ; i++) {
> +    res+=text;
> +  }
> +  return res;
> +}

You could also use Array(num + 1).join(text) for this

@@ +466,5 @@
> +    btn.appendChild(document.createTextNode("Copy"));
> +    btn.histogramText = aName + "\n" + textData;
> +    btn.onclick = function(){toClipboard(this.histogramText);};
> +    outerDiv.appendChild(btn);
> +*/

We're not supposed to set innerHTML or use onclick

@@ +536,5 @@
>      for (let [label, value] of aValues) {
> +      // Create a text representation of the bar (label | bar-of-* (value) percentage )
> +      text += "\n" + repeat(" ", labelPadTo - String(label).length) + label
> +                   + " |" + (!aMaxValue ? "X" : repeat("*", Math.round(MAX_BAR_CHARS * value / aMaxValue)))
> +                   + " " + value + "  " + (!aSampleCount ? "X" : Math.round(100 * value / aSampleCount)) + "%";

Not very readable :) Put this in a separate function
Attachment #8339015 - Flags: review?(vdjeric)
Modified the output to look slightly less noisy (making the percentage value pop out more clearly, and hide-ish the value itself which is less important):

HTTP_SUB_COMPLETE_LOAD
366 samples, average = 292.9, sum = 107219

   7 |0  0%
   9 |#2  1%
  52 |1  0%
  63 |#3  1%
  77 |1  0%
  94 |#4  1%
 115 |#######22  6%
 140 |################48  13%
 171 |#########################77  21%
 209 |#######################70  19%
 255 |##############44  12%
 311 |#####16  4%
 379 |####11  3%
 462 |#######23  6%
 564 |#######23  6%
 688 |####13  4%
 839 |###8  2%
1023 |0  0%

Addressed all comments except moving the textual bar builder to a new function, but I did simplify it a bit by removing the error checking (the rest of the code assumes the same, so I commented on that too).
Attachment #8339015 - Attachment is obsolete: true
Attachment #8340523 - Flags: review?(vdjeric)
Attachment #8340523 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/ec86d240ceed
Assignee: nobody → avihpit
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [mentor=vladan][lang=js][good first bug] → [mentor=vladan][lang=js][good first bug][good first verify]
You need to log in before you can comment on or make changes to this bug.