Closed Bug 1040079 Opened 6 years ago Closed 6 years ago

Adjust logging levels for devicemanager*.py

Categories

(Testing :: Mozbase, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla38

People

(Reporter: armenzg, Assigned: armenzg, Mentored)

Details

Attachments

(1 file, 1 obsolete file)

This seems like an easy first bug.

The intention of this bug is to get informative messages into the tbpl logs and determine if on mozdevice we should changes some messages to the right level.

The steps to follow would be:
* change the default mozlog.ERROR to mozlog.INFO [1]
* review the logging messages and see if they need to be change between info/debug/error
** change directory to testing/mozbase/mozdevice/mozdevice
** run "grep -E "(info|debug|error)\(" devicemanager*py" to find all instances
* attach your patch and request me to have a look
** once we have a good patch we can test it on try
* we can push to try with try: -b o -p linux64,android,emulator -u xpcshell,mochitest-1,crashtest-1,reftest-1,marionette-webapi -t none
** that would generate a series of jobs that would put your changes to the test
** you would have to have a 

[1] http://hg.mozilla.org/mozilla-central/file/f92e4d13b3c6/testing/mozbase/mozdevice/mozdevice/devicemanager.py#l49
Mentor: armenzg
Whiteboard: [good first bug]
This is not too difficult, but is probably a bit involved for a good first bug. Let's make it a good next bug.
Whiteboard: [good first bug] → [good next bug]
Hi Armen, 

I would like to work on this bug. Can you please assign it to me?
Attached patch 1040079.patch (obsolete) — Splinter Review
Hi Armen, 

I have followed all the steps that you have given. Do let me know if what I have done is what you wanted. 

Thank you.
Attachment #8477934 - Flags: review?(armenzg)
Hello abhi12ravi,
My apologies for the late reply. I've been out for holidays.
I should be able to look into this with detail on Tuesday.

Thanks for the patch!
Comment on attachment 8477934 [details] [diff] [review]
1040079.patch

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

Thanks abhi12ravi for the patch. Overall the patch is heading on the right direction, however, somethings have changed since the bug got filed. I need to figure those out.
Attachment #8477934 - Flags: review?(armenzg)
Hi wlach,
Since I filed the bug it seems that few things have changed.
At this point, we don't have a default logLevel as we used to [1].
From looking at tbpl, I can't see any output of mozdevice anymore.
I know that jgraham was excited to be able to see so much more output of mozdevice since it allowed him to know what was going on.

I think we have to discuss what we did on bug 1041533 and see if we have to change anything.
Otherwise we would have to go around the tree and have to set a level for each call of DeviceManagerADB.

What do you think?

On the other hand, this is not blocking anything but rather trying to improve our logging status.

[1] http://hg.mozilla.org/mozilla-central/rev/10dc968f23ea
Flags: needinfo?(wlachance)
Hey Armen, sorry about not giving feedback on this earlier.

I don't want to revisit bug 1041533, as that approach caused some pretty weird behaviour that was hard to track down. Also, I think most of the existing logging in mozdevice.DeviceManagerADB is at about the right level, I don't think we should change debug->info in most of the cases of abhi12ravi@gmail.com's patch, the only thing that stuck out as the right thing was maybe the reverse switch from info->debug in the chmod calls.

I think at this point it would be best to focus on making mozdevice work well with structured logging, it's not really clear to me that further adjustments on how we're doing old-style logging in tree would provide a good return on time invested at this point. I suspect James or Chris would be the best people to ask about that.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(wlachance)
Whiteboard: [good next bug]
Hey Armen and William, 

Thanks for taking a look at my patch. Do I wait till you guys figure out what's supposed to be done? Learning about the log levels was interesting, thanks for that!
Hey Armen, I think I'd like to just apply the two changes I mentioned in comment 7 and then close out this bug. Is that ok with you?
Flags: needinfo?(armenzg)
(In reply to William Lachance (:wlach) from comment #9)
> Hey Armen, I think I'd like to just apply the two changes I mentioned in
> comment 7 and then close out this bug. Is that ok with you?

Yes, both proposed changes make change. Thanks for keeping this alive!
Flags: needinfo?(armenzg)
armen, can you wrap this up?
Flags: needinfo?(armenzg)
Assignee: nobody → armenzg
Attachment #8477934 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(armenzg)
Attachment #8555406 - Flags: review?(wlachance)
Comment on attachment 8555406 [details] [diff] [review]
change chmod from info to debug

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

lgtm
Attachment #8555406 - Flags: review?(wlachance) → review+
https://hg.mozilla.org/mozilla-central/rev/fabb44ffd5d0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.