Closed Bug 1079763 Opened 6 years ago Closed 5 years ago

Compress logs produced by LogShake

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-master verified)

VERIFIED FIXED
Tracking Status
b2g-master --- verified

People

(Reporter: gerard-majax, Assigned: hobinjk)

References

Details

Attachments

(2 files, 5 obsolete files)

We should be able to produce a zip file of the logs, to limit the consumed space on the sdcard and to ease sharing
Flags: needinfo?(jlorenzo)
Discussed offline. No need to provide any info.
Flags: needinfo?(jlorenzo)
See Also: → 1173777
Assignee: nobody → hobinjk
Attached patch Gecko patch part one (obsolete) — Splinter Review
This patch has no functional changes but instead refactors parts of LogShake to be less indented and hard to read.
Attached patch Gecko patch part two (obsolete) — Splinter Review
This is the actual implementation of compression. LogShake now stores every log in logArrays into a single zip archive at location zipFilename. The message replaces logPrefix with zipFilename. logFilenames now refers to the files present in the archive.

The gaia implementation will be done later today.
Attached patch Gecko patch part two - Minor fix (obsolete) — Splinter Review
Fixes one instance where logPrefix was not replaced by zipFilename
Attachment #8621844 - Attachment is obsolete: true
Attached file Gaia pull request
PR adding support for this in Gaia.
Attachment #8621841 - Flags: review?(lissyx+mozillians)
Attachment #8621866 - Flags: review?(lissyx+mozillians)
Attachment #8621884 - Flags: review?(lissyx+mozillians)
Dale, I suggest we make this an optional feature and not enabled by default, otherwise it may break your work regarding filtering of sensitive data.
Flags: needinfo?(dale)
Attachment #8621841 - Flags: review?(lissyx+mozillians)
Comment on attachment 8621866 [details] [diff] [review]
Gecko patch part two - Minor fix

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

Please make it just one patch if you are doing changes on top of other changes: it's too messy otherwise.

::: b2g/components/LogShake.jsm
@@ +388,5 @@
>      dirName = getLogDirectory();
>    } catch(e) {
>      // Return promise failed with exception e
>      // Handles missing sdcard
> +    return Promise.reject("Getting directories failed: " + e.message);

I prefer we keep passing objects instead of error messages
Attachment #8621866 - Flags: review?(lissyx+mozillians)
> Dale, I suggest we make this an optional feature and not enabled by default, otherwise 
> it may break your work regarding filtering of sensitive data.

Yeh we would really like to give the user the ability to view what data was captured before sending it off, in particular the screenshot to see if it even captures the bug so this would break a fairly important part of our use case.

But if its off by default and the format of the activity data doesnt change then completely fine by me.
Flags: needinfo?(dale)
Oh nice! Privacy features are a much-needed addition. I think I'll wait on landing these changes until after the sensitive filtering data is in place.

Dale, if you want any assistance with the filtering feature I have spare cycles to spend.
So I am working on https://bugzilla.mozilla.org/show_bug.cgi?id=1159969 and should be finished today so all good there, but there is more work that can be done and would love to help guide any contribution https://bugzilla.mozilla.org/show_bug.cgi?id=1155418 for example is a bug filed to help users preview the data they are sending (currently only images work) it would be awesome to improve this and expand it to more file types, very happy to help out if you would like to work on it (can needinfo me or daleharvey on #gaia / #b2g)
Attached patch bug1079763-gecko.patch (obsolete) — Splinter Review
Attachment #8621841 - Attachment is obsolete: true
Attachment #8621866 - Attachment is obsolete: true
Attachment #8627984 - Flags: review?(lissyx+mozillians)
Comment on attachment 8627984 [details] [diff] [review]
bug1079763-gecko.patch

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

Nice work, I'd like that we cleanup settings.js though :)

::: b2g/chrome/content/settings.js
@@ +205,4 @@
>    LogShake.init();
>  })();
>  
> +SettingsListener.observe('devtools.logshake.enabled', false, value => {

Could we try to move everything related to logshake out of settings.js ? I feel we don't really need this here: we can still make use of settings service to query the observers.

::: b2g/components/LogShake.jsm
@@ +252,4 @@
>      this.captureLogs().then(logResults => {
>        // On resolution send the success event to the requester
>        SystemAppProxy._sendCustomEvent(CAPTURE_LOGS_SUCCESS_EVENT, {
> +        logPaths: logResults.logPaths,

We are just renaming for consistency here, right? But we will have to update Gaia-side as well :(

::: b2g/components/test/unit/test_logshake_gonk_compression.js
@@ +71,5 @@
> +  let expectedFiles = [];
> +
> +  LogShake.enableQAMode();
> +
> +  LogShake.captureLogs().then(logResults => {

Looks good but I fear of timeouts like we had previously :(
Attachment #8627984 - Flags: review?(lissyx+mozillians) → review+
I decided that having a consistent and correct naming scheme that properly reflects the difference between compressed and uncompressed log files was worth the renaming. The reason for introducing the new test_logshake_gonk_compression is to (hopefully) avoid the timeouts by having two separate tests. I believe that the timeout is per test file, meaning that even though test_logshake_gonk + test_logshake_gonk_compression would time out, the two separately will not.

I'm not sure how to break the dependence on SettingsListener. It's what the Developer HUD uses in a very similar way. One thing that is missing is lazy-loading of LogShake, but I can incorporate that if desired.
Flags: needinfo?(lissyx+mozillians)
(In reply to James Hobin [:hobinjk] from comment #13)
> I decided that having a consistent and correct naming scheme that properly
> reflects the difference between compressed and uncompressed log files was
> worth the renaming. The reason for introducing the new
> test_logshake_gonk_compression is to (hopefully) avoid the timeouts by
> having two separate tests. I believe that the timeout is per test file,
> meaning that even though test_logshake_gonk + test_logshake_gonk_compression
> would time out, the two separately will not.

Perfect then :)

> 
> I'm not sure how to break the dependence on SettingsListener. It's what the
> Developer HUD uses in a very similar way. One thing that is missing is
> lazy-loading of LogShake, but I can incorporate that if desired.

As far as I can read the current code, we do load LogShake.jsm everytime, we just make it listen to events upon change of the settings value. 

We could just load logshake.jsm and do nothing, and enable the feature after getting a callback from the settings value with this: https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsService.js#264
Flags: needinfo?(lissyx+mozillians) → needinfo?(hobinjk)
(In reply to Alexandre LISSY :gerard-majax from comment #14)
> (In reply to James Hobin [:hobinjk] from comment #13)
> > 
> > I'm not sure how to break the dependence on SettingsListener. It's what the
> > Developer HUD uses in a very similar way. One thing that is missing is
> > lazy-loading of LogShake, but I can incorporate that if desired.
> 
> As far as I can read the current code, we do load LogShake.jsm everytime, we
> just make it listen to events upon change of the settings value. 
> 
> We could just load logshake.jsm and do nothing, and enable the feature after
> getting a callback from the settings value with this:
> https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsService.
> js#264

Funnily enough, this settings-callback-based approach is the way the original LogShake was implemented. At the time I changed it due to errors in test runs. For me there are a few reasons why I prefer the current approach.

The foremost is that it doesn't duplicate code. The part of LogShake that deals with settings is entirely handled by a separate component.The second is that it follows what the Developer HUD uses. The final reason is that it is already implemented and tested.

Switching LogShake to handle all of its settings logic is a potentially good idea from the perspective of making sure that more LogShake-related code is in a central location. However, the consolidated approach would still need to be initialized in b2g/chrome/content/shell.js or an equivalent.

I took the time to create a branch with the desired changes. It appears to work flawlessly except for in tests, where it test_logshake.js reliably causes a PROCESS-CRASH event.
Flags: needinfo?(hobinjk)
Right, let's go ahead with the current solution then.
Comment on attachment 8621884 [details] [review]
Gaia pull request

Looks ok, but I don't see tests for QA mode ?
Flags: needinfo?(hobinjk)
Attachment #8621884 - Flags: review?(lissyx+mozillians) → review+
I accidentally deleted all of my tests involving compression, the pull request should now have them included.
Flags: needinfo?(hobinjk)
Attached patch Gecko patch (obsolete) — Splinter Review
This doesn't actually change anything other than swapping a var for let that was introduced in a different patch. I'm flagging for re-review because of the conversation surrounding SettingsService.
Attachment #8627984 - Attachment is obsolete: true
Attachment #8630595 - Flags: review?(lissyx+mozillians)
For some reason I can't re-trigger the job which encountered an intermittent failure on the Gaia PR. The try build for the gecko changes is available here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=44602e3c284f
Attached patch bug1079763.patchSplinter Review
Now running at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d50f89268f4e because previous patch didn't include the new test file.
Attachment #8630595 - Attachment is obsolete: true
Attachment #8630595 - Flags: review?(lissyx+mozillians)
Attachment #8630782 - Flags: review?(lissyx+mozillians)
Comment on attachment 8630782 [details] [diff] [review]
bug1079763.patch

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

::: b2g/components/LogShake.jsm
@@ +534,5 @@
> +    let logFilenames = [];
> +    let logPaths = [];
> +    let saveRequests = [];
> +
> +    for (let logLocation in logArrays) {

Can we make sure that one potential failure during the write will not stop others ?
Attachment #8630782 - Flags: review?(lissyx+mozillians) → review+
Comment on attachment 8621884 [details] [review]
Gaia pull request

I think we would want some test coverage or toggling the qa_enabled setting?
Flags: needinfo?(hobinjk)
(In reply to Alexandre LISSY :gerard-majax from comment #22)
> Comment on attachment 8630782 [details] [diff] [review]
> bug1079763.patch
> 
> Review of attachment 8630782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/LogShake.jsm
> @@ +534,5 @@
> > +    let logFilenames = [];
> > +    let logPaths = [];
> > +    let saveRequests = [];
> > +
> > +    for (let logLocation in logArrays) {
> 
> Can we make sure that one potential failure during the write will not stop
> others ?

That is what currently happens due to how Promises work with Promise.all. Currently, if an error occurs it will be reported, but all files in the log directory other than the one that caused the error will exist.

I could create a threshold for reporting errors, but I'm not sure what a good value for that would be.

(In reply to Alexandre LISSY :gerard-majax from comment #23)
> Comment on attachment 8621884 [details] [review]
> Gaia pull request
> 
> I think we would want some test coverage or toggling the qa_enabled setting?

As far as I can tell there are no tests of settings implemented in the standard way
Flags: needinfo?(hobinjk)
Ship it!
Keywords: checkin-needed
looks like this landed without being notified?
https://hg.mozilla.org/mozilla-central/rev/d10e59492245
https://github.com/mozilla-b2g/gaia/commit/1bd8fd6522c1fed6f113858035fb76cb68d59a46
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This issue is verified fixed on the latest Spark and Flame 2.5 builds.
Logs produced by LogShake are properly compressed into a .zip file.

Environmental Variables:
Device: Flame 2.5
Build ID: 20150810030209
Gaia: 09dea2d5ff21cdb56da35fe4aa5bf4c90cf1da7f
Gecko: 0e269a1f1beb
Gonk: c4779d6da0f85894b1f78f0351b43f2949e8decd
Version: 42.0a1 (2.5)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0

Environmental Variables:
Device: Aries 2.5
Build ID: 20150810142756
Gaia: fa89e03dc489e79baa0e74cb1d205260c7924caa
Gecko: cd45a38ded04
Gonk: 2916e2368074b5383c80bf5a0fba3fc83ba310bd
Version: 42.0a1 (2.5)
Firmware Version: D5803_23.1.A.1.28_NCB.ftf
User Agent: Mozilla/5.0 (Mobile; rv:42.0) Gecko/42.0 Firefox/42.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Duplicate of this bug: 1173777
You need to log in before you can comment on or make changes to this bug.