Closed
Bug 1329227
Opened 7 years ago
Closed 7 years ago
Put disqus comment information in Histograms.json descriptions
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: chutten, Assigned: amiyaguchi, Mentored)
References
Details
(Whiteboard: [good first bug] [lang=js])
Attachments
(2 files, 2 obsolete files)
933 bytes,
text/plain
|
Details | |
1.01 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•7 years ago
|
||
To check if there are any other, this lists the TMO forums: https://disqus.com/home/forums/telemetrydashboards/
Comment 2•7 years ago
|
||
All relevant disqus comments.
Reporter | ||
Comment 3•7 years 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]
Updated•7 years ago
|
Component: Telemetry Dashboard → Telemetry
Product: Webtools → Toolkit
Whiteboard: [good first bug] → [good first bug] [lang=js]
Comment 4•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
For now let's leave WEBRTC_ICE_FINAL_CONNECTION_STATE alone. If it becomes relevant later it can be added later.
Comment 10•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Attachment #8868235 -
Flags: review?(chutten)
Reporter | ||
Comment 16•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Push the updated patch to mozreview and I'll take a look
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(mstokes13)
Comment 22•7 years 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•7 years 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•7 years ago
|
Flags: needinfo?(mstokes13)
Reporter | ||
Comment 24•7 years ago
|
||
This bug appears to be idle. A few changes to the patch should make this bug an easy fix.
Updated•7 years ago
|
Assignee: nobody → amiyaguchi
Assignee | ||
Comment 25•7 years ago
|
||
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 26•7 years ago
|
||
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•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Attachment #8866446 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8868235 -
Attachment is obsolete: true
Comment 27•7 years 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
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77c33119b5ca
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•