Put disqus comment information in Histograms.json descriptions

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: chutten, Assigned: amiyaguchi, Mentored)

Tracking

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
There are a few useful disqus comments on tmo:

http://disq.us/p/10ldbqx
http://disq.us/p/10lem9p
http://disq.us/p/10l4qsj
http://disq.us/p/10gxa1y

This information should be recorded in Histograms.json before we decommission disqus.
To check if there are any other, this lists the TMO forums:
https://disqus.com/home/forums/telemetrydashboards/
Created attachment 8824994 [details]
disqus.txt

All relevant disqus comments.
(Reporter)

Comment 3

a year ago
This should be a good first bug: rewrite the contents of disqus.txt into the appropriate Histograms.json description fields.
Mentor: chutten
Priority: -- → P3
Whiteboard: [good first bug]
Component: Telemetry Dashboard → Telemetry
Product: Webtools → Toolkit
Whiteboard: [good first bug] → [good first bug] [lang=js]

Comment 4

a year ago
Hello, I would like to work on this bug. The links that you added that you said are a few useful disqus comment on tmo, when opening do not seem to be working correctly.
(Reporter)

Comment 5

a year ago
The relevant disqus comments were removed from the website and attached as a text file to this bug.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8866446 [details]
Bug 1329227 - 'Added comments to Histograms.json file as per request' <chutten>

https://reviewboard.mozilla.org/r/138070/#review141250

::: commit-message-2b6a6:1
(Diff revision 1)
> +Bug 1329227 - 'Added comments to Histograms.json file as per request' r=<chutten>

To correctly identify me as the requested reviewer, use `r?chutten`

While we're here, "as per request" is not useful information to include in the first line of a commit message. A good commit message for mozilla-central would be more like

```
Bug 1329227 - Added disqus comments to histograms' descriptions r?chutten

After disqus was removed from telemetry.mozilla.org we lost some context from those comments.
This puts that context into the descriptions of the relevant histograms.
```

You did very well with using the correct tense in the topline summary. Most people don't get that right.

::: toolkit/components/telemetry/Histograms.json:3265
(Diff revision 1)
>      "kind": "linear",
>      "high": 21,
>      "n_buckets": 20,
>      "description": "The number of unusable addresses reported for each record"
>    },
> +  "LOOP_ICE_FINAL_CONNECTION_STATE":{

Instead of writing new probe definitions like this you should be reformatting the data into the descriptions of the probe definitions already present in this file.

If the probe has been removed, then there's no need to add a description.

::: toolkit/components/telemetry/Histograms.json:3292
(Diff revision 1)
> +        "2": "Mixed active content",
> +        "3": "Both mixed passive and mixed active content"
> +    },
> +    "SSL_CIPHER_SUITE_FULL":{
> +    "Entries #8, 9, 47, 48, 68, and 69 correspond to ciphers that use RC4."
> +    },

This file might fail linting with these changes, and certainly won't build (the comments you introduced here will be interpreted as illegal histogram definitions which `./mach build` will reject)

Please remember to run `./mach lint toolkit/componenets/telemetry` and `./mach build` to ensure your changes don't fail static analysis or the build.
Attachment #8866446 - Flags: review-

Comment 8

a year ago
Hey Chris, we couldn't find the LOOP_CANDIDATE_TYPES_GIVEN_SUCCESS and LOOP_ICE_FINAL_CONNECTION_STATE. There is WEBRTC_ICE_FINAL_CONNECTION_STATE, and we were wondering if the comment LOOP_ICE_FINAL_CONNECTION_STATE for go under this one?

Thanks for the help! We are still pretty green at this.
(Reporter)

Comment 9

a year ago
For now let's leave WEBRTC_ICE_FINAL_CONNECTION_STATE alone. If it becomes relevant later it can be added later.

Comment 10

a year ago
(In reply to Chris H-C :chutten from comment #9)
> For now let's leave WEBRTC_ICE_FINAL_CONNECTION_STATE alone. If it becomes
> relevant later it can be added later.

Ok so we added the stuff to the ones that did exist. We will run the lint toolkit and see if it builds. Sorry about the as per request comment. A couple of other advisers asked for it in the comments.

Comment 11

a year ago
(In reply to Chris H-C :chutten from comment #9)
> For now let's leave WEBRTC_ICE_FINAL_CONNECTION_STATE alone. If it becomes
> relevant later it can be added later.

Hey Chris, we went to run lint and got this message. I have Node installed in the bin. I figured its just a matter of exporting the path so the mozilla source knows where it is. Or is it something else.

--------------------------------------------------------------------------------------------------------------
nodejs v6.9.1 is either not installed or is installed to a non-standard path.
Please install nodejs from https://nodejs.org and try again.

Valid installation paths:
  - /usr/bin/nodejs
DEBUG:manifest:Opening manifest at /home/aaron/src/mozilla-central/testing/web-platform/meta/MANIFEST.json
^CProcess PoolWorker-3:
Traceback (most recent call last):
  File "/usr/lib/python2.7/multiprocessing/process.py", line 258, in _bootstrap
    self.run()
  File "/usr/lib/python2.7/multiprocessing/process.py", line 114, in run
    self._target(*self._args, **self._kwargs)
  File "/usr/lib/python2.7/multiprocessing/pool.py", line 102, in worker
    task = get()
  File "/usr/lib/python2.7/multiprocessing/queues.py", line 378, in get
    return recv()
KeyboardInterrupt
(Reporter)

Comment 12

a year ago
Do you have node or nodejs? I know some environments confuse the two. Double-check with `which nodejs` to ensure it's where you think it is.

If you're on IRC (and I recommend you do so: https://wiki.mozilla.org/Irc ) the #developers or #introduction channel might be able to help you out with getting linting up and running.

Comment 13

11 months ago
Ok. As far as I know it is nodejs and it is installed in the usr/bin like usual. I will see if I can find something out.

Comment 14

11 months ago
Ok, we ran lint and it said no errors. We ran the build and got some errors. I wasn't sure if they were related to what we are working on.

I attached the errors in a text file under the attachments tab.

Thanks for the patience!

Comment 15

11 months ago
Created attachment 8868235 [details]
buildrun.txt
Attachment #8868235 - Flags: review?(chutten)
(Reporter)

Comment 16

11 months ago
Comment on attachment 8868235 [details]
buildrun.txt

Seems as though there's a problem at line 9110 column 3 of your Histograms.json. Might be missing a comma, or having too many commas, or missing a colon, or a field name might not be quoted.
Attachment #8868235 - Flags: review?(chutten)

Comment 17

11 months ago
(In reply to Chris H-C :chutten from comment #16)
> Comment on attachment 8868235 [details]
> buildrun.txt
> 
> Seems as though there's a problem at line 9110 column 3 of your
> Histograms.json. Might be missing a comma, or having too many commas, or
> missing a colon, or a field name might not be quoted.

Hey Chris, quick question. When we use./mach build can we specify the file path we are working on or do we have to build the entire mozilla-central every time?

Comment 18

11 months ago
Hey Chris we got most of our stuff to build but we get an error that 0,1,2,3 not allowed for MIXED_CONTENT_PAGE_LOAD with these comments:

  "MIXED_CONTENT_PAGE_LOAD": {
    "record_in_processes": ["main", "content"],
    "expires_in_version": "never",
    "kind": "enumerated",
    "n_values": 4,
    "description": "Accumulates type of content per page load (0=no mixed or non-secure page, 1=mixed passive, 2=mixed active, 3=mixed passive and mixed active)",
    "0": "No mixed content",
    "1": "Mixed passive content",
    "2": "Mixed active content",
    "3": "Both mixed passive and mixed active content"
  },

Should we leave these out?
(Reporter)

Comment 19

11 months ago
The purpose of this bug is to include the meaning of the comments into the "description" field of the related probes.

In this case, the meaning is already present in the description, so no changes are needed.

To your earlier question about building just part, yes you can. `mach build toolkit/components/telemetry` should build what you're after.

Comment 20

11 months ago
(In reply to Chris H-C :chutten from comment #19)
> The purpose of this bug is to include the meaning of the comments into the
> "description" field of the related probes.
> 
> In this case, the meaning is already present in the description, so no
> changes are needed.
> 
> To your earlier question about building just part, yes you can. `mach build
> toolkit/components/telemetry` should build what you're after.

Great thanks, it seemed a little excessive to build the entire source every time I changed something. So if no changes are needed for the description on MIXED_CONTENT_PAGE_LOAD then we have only made one change and are ready to push the patch back.
(Reporter)

Comment 21

11 months ago
Push the updated patch to mozreview and I'll take a look
(Reporter)

Updated

11 months ago
Flags: needinfo?(mstokes13)

Comment 22

11 months ago
(In reply to Chris H-C :chutten from comment #21)
> Push the updated patch to mozreview and I'll take a look

Hey Chris, sorry for the wait. We have been having some difficulty with mercurial. For some reason every time we got to push the patch it creates an empty changset even though the committed changes are listed in the patch. If we can't figure it out in a week or two we may ask to be unassigned from this bug. We were doing this for a school project and the term is almost over.

Thanks for all the help!

- Aaron, Ahmed, Abby, and Mason
(Reporter)

Comment 23

11 months ago
If it's giving you problems interacting with mozreview, you could try generating a patch file and attaching it here to this bug instead. The internet tells me (I use git, my hg is weak) the way to do this is `hg export -o FILE -r REV`

That should give you a file you can then attach here and set a r?chutten flag on (you can mark the old review as obsolete on the Attach File page if you want.)
(Reporter)

Updated

10 months ago
Flags: needinfo?(mstokes13)
(Reporter)

Comment 24

10 months ago
This bug appears to be idle. A few changes to the patch should make this bug an easy fix.
Assignee: nobody → amiyaguchi
(Assignee)

Comment 25

9 months ago
Created attachment 8892033 [details] [diff] [review]
Bug-1329227-Discus-comments.patch

This adds all the relevant comments from discus. `LOOP_*` probes are no longer listed. `MIXED_CONTENT_PAGE_LOAD` already contains comments from these comments.
Attachment #8892033 - Flags: review?(gfritzsche)
Comment on attachment 8892033 [details] [diff] [review]
Bug-1329227-Discus-comments.patch

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

Looks good to me, thanks!
Attachment #8892033 - Flags: review?(gfritzsche) → review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed
Attachment #8866446 - Attachment is obsolete: true
Attachment #8868235 - Attachment is obsolete: true

Comment 27

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77c33119b5ca
Put disqus comment information in Histograms.json descriptions. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/77c33119b5ca
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.