Repository: google/mobly

Issue:
[Android Device] Identify the device in exceptions thrown from a device instance.
Right now we have to manually add the device serial every time we throw an exception from inside AndroidDevice, which is tedious and annoying.
We should have a better way to handle this, perhaps an inner exception class.

Discussion:
adorokhine: 



Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

*[mobly/controllers/android_device.py, line 155 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVCw_pSPnznkgrgh2R:-KcVCw_pSPnznkgrgh2S:bp84v0n) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L155)):*
> ```Python
>             else:
>                 ad.log.exception('Skipping this optional device because some '
>                                  'services failed to start: %s', e)
> ```

for ad.log.exception, exception is automatically logged so don't need %s, e

---

*[mobly/controllers/android_device.py, line 242 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDCYP1feDwGCUUqvJ:-KcVDCYP1feDwGCUUqvK:bnrq4kl) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L242)):*
> ```Python
>             if is_required:
>                 raise
>             ad.log.warning('Skipping this optional device due to error.')
> ```

You no longer need to say what error it was?

---

*[mobly/controllers/android_device.py, line 376 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDfbkUdqpf3kzfjrO:-KcVDfblO3BELNVkZXmf:bleaqiq) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L376)):*
> ```Python
>         self.log = AndroidDeviceLoggerAdapter(logging.getLogger(),
>                                               {'tag': self.serial})
>         self.Error = _generate_error_class(self.log.extra['tag'])
> ```

How do we extend this? ie to differentiate bluetooth_utils.Error('Device failed to connect') from wifi_utils.Error('Device failed to connect')?

---

*[mobly/controllers/android_device.py, line 394 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDrQVJzkuY5hYCRvK:-KcVDrQWBf3vvpGCtl44:b9kun1o) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L394)):*
> ```Python
> 
>         By default, the tag is the serial of the device, but sometimes it may
>         be more descriptive to use a different tag of the user's choose.
> ```

The user's choose

---

*[mobly/controllers/android_device.py, line 399 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDunSLydX9J8zUmvx:-KcVDunTNMJzCxXETSMi:bpstgh1) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L399)):*
> ```Python
>             By default, the device's serial number is used:
>                 'INFO [AndroidDevice|abcdefg12345] One pending call ringing.'
>             By calling `ad.set_prefix_tag('Caller')`, the user can customize the
> ```

Doesn't match the actual method name

---

*[mobly/controllers/android_device.py, line 516 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVEA9hEnvBdvpK7Uy0:-KcVEA9i99rP1NP-sBx6:b-gwoogj) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L516)):*
> ```Python
>             if hasattr(self, k):
>                 raise self.Error(
>                     'Attribute %s already exists, cannot set again.')
> ```

%s not formatted?

---

*[mobly/controllers/android_device.py, line 548 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVEFCMxb274QhQcAZk:-KcVEFCMxb274QhQcAZl:buqy7ru) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L548)):*
> ```Python
>         # Should not load snippet with the same attribute more than once.
>         if name in self._snippet_clients:
>             raise self.Error(
> ```

We're now no longer telling the user this was a SnippetError. Can we have that back? That was useful info

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

*[mobly/controllers/android_device.py, line 155 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVCw_pSPnznkgrgh2R:-KcVIGVZ1M0fEhtj8uHg:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L155)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

for ad.log.exception, exception is automatically logged so don't need %s, e
</blockquote></details>

Done

---

*[mobly/controllers/android_device.py, line 242 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDCYP1feDwGCUUqvJ:-KcVIHA27Gfg84-lzuEe:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L242)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

You no longer need to say what error it was?
</blockquote></details>

Done

---

*[mobly/controllers/android_device.py, line 376 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDfbkUdqpf3kzfjrO:-KcVII98a20SRjzLUpPf:b-eo07wu) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L376)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

How do we extend this? ie to differentiate bluetooth_utils.Error('Device failed to connect') from wifi_utils.Error('Device failed to connect')?
</blockquote></details>

How do we differentiate today? Doesn't seem to be a problem specific to this change?

---

*[mobly/controllers/android_device.py, line 394 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDrQVJzkuY5hYCRvK:-KcVI_MWa3Jpzx6ykOBi:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L394)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

The user's choose
</blockquote></details>

Done

---

*[mobly/controllers/android_device.py, line 399 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVDunSLydX9J8zUmvx:-KcVId1q8qOl7N3Jxw2l:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L399)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Doesn't match the actual method name
</blockquote></details>

Done

---

*[mobly/controllers/android_device.py, line 516 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVEA9hEnvBdvpK7Uy0:-KcVIrgY5f6BwcnuLuqN:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L516)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

%s not formatted?
</blockquote></details>

Done

---

*[mobly/controllers/android_device.py, line 548 at r1](https://reviewable.io:443/reviews/google/mobly/107#-KcVEFCMxb274QhQcAZk:-KcVNHQV5CbkJ4abKyvA:bb6un1j) ([raw file](https://github.com/google/mobly/blob/cc3837b019da67485846e10e4b65d2a80c7779b1/mobly/controllers/android_device.py#L548)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

We're now no longer telling the user this was a SnippetError. Can we have that back? That was useful info
</blockquote></details>

Done

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

xpconanfan: Ping
adorokhine: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 55 at r3](https://reviewable.io:443/reviews/google/mobly/107#-Keb5oHRIXDbUc-naq49:-Keb5oHS8h-BZPo1EXCv:b-f06rqw) ([raw file](https://github.com/google/mobly/blob/d5e96ea3a401277d359d8c1456785b92a0b63067/mobly/controllers/android_device.py#L55)):*
> ```Python
> 
> 
> def _generate_error_class(tag, error_name):
> ```

Instead of doing this complex exception class manipulation which doesn't help users, can we just change `__repr__` to the log tag so you can throw it into arbitrary exceptions?
`raise MyException(ad, 'Something bad happened')`

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 55 at r3](https://reviewable.io:443/reviews/google/mobly/107#-Keb5oHRIXDbUc-naq49:-KebAO3RWIqxxqQPPpfB:bhhmiss) ([raw file](https://github.com/google/mobly/blob/d5e96ea3a401277d359d8c1456785b92a0b63067/mobly/controllers/android_device.py#L55)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Instead of doing this complex exception class manipulation which doesn't help users, can we just change `__repr__` to the log tag so you can throw it into arbitrary exceptions?
`raise MyException(ad, 'Something bad happened')`
</blockquote></details>

I tried and we can't.
`repr` value does not change.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

adorokhine: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 55 at r3](https://reviewable.io:443/reviews/google/mobly/107#-Keb5oHRIXDbUc-naq49:-KebOVg6c_pBFQA-l3k-:b-jhayew) ([raw file](https://github.com/google/mobly/blob/d5e96ea3a401277d359d8c1456785b92a0b63067/mobly/controllers/android_device.py#L55)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

I tried and we can't.
`repr` value does not change.
</blockquote></details>

I'm not sure what you mean by 'repr value does not change'? Mind sharing a code snippet of what you tried?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 55 at r3](https://reviewable.io:443/reviews/google/mobly/107#-Keb5oHRIXDbUc-naq49:-KebSuS1J4wwPaF5NQbK:b-ie466z) ([raw file](https://github.com/google/mobly/blob/d5e96ea3a401277d359d8c1456785b92a0b63067/mobly/controllers/android_device.py#L55)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

I'm not sure what you mean by 'repr value does not change'? Mind sharing a code snippet of what you tried?
</blockquote></details>

Nvm, there was a bug in the code I used to try.
Done

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

adorokhine: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 53 at r4](https://reviewable.io:443/reviews/google/mobly/107#-KeeYOfb4WvMXMwaepwT:-KeeYOfcs0OskUJpoPSt:buscggs) ([raw file](https://github.com/google/mobly/blob/c287393948cb53d2fedfcfd062092705578f593e/mobly/controllers/android_device.py#L53)):*
> ```Python
> 
> 
> class DeviceError(Error):
> ```

DeviceError shouldn't be necessary at all after the __repr__ implementation, so let's simplify by removing it and just throwing a regular exception like users would do it. It'll be a good example to follow.

---

*[mobly/controllers/android_device.py, line 408 at r4](https://reviewable.io:443/reviews/google/mobly/107#-KeeYChvRdxHpIQ4RRik:-KeeYChwGzsdZ85Fpw19:bnn81je) ([raw file](https://github.com/google/mobly/blob/c287393948cb53d2fedfcfd062092705578f593e/mobly/controllers/android_device.py#L408)):*
> ```Python
>             By default, the device's serial number is used:
>                 'INFO [AndroidDevice|abcdefg12345] One pending call ringing.'
>             By calling `ad.set_debug_tag('Caller')`, the user can customize the
> ```

Is it set_debug_tag? Or just debug_tag('caller')?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 6 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 53 at r4](https://reviewable.io:443/reviews/google/mobly/107#-KeeYOfb4WvMXMwaepwT:-KeeZDHIj56_yGLokelk:b-ho4n7x) ([raw file](https://github.com/google/mobly/blob/c287393948cb53d2fedfcfd062092705578f593e/mobly/controllers/android_device.py#L53)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

DeviceError shouldn't be necessary at all after the __repr__ implementation, so let's simplify by removing it and just throwing a regular exception like users would do it. It'll be a good example to follow.
</blockquote></details>

I specifically want this error class so the msg is easier to read.
The default repr msg is quite ugly since it's just a tuple of reprs.
I want msg from AndroidDevice object to be cleaner than that.

Also this error serves as a distinction between errors from `android_device` module and `AndroidDevice` class.

---

*[mobly/controllers/android_device.py, line 408 at r4](https://reviewable.io:443/reviews/google/mobly/107#-KeeYChvRdxHpIQ4RRik:-Kee_DhNzRaTFzGFT9Ru:b-896fix) ([raw file](https://github.com/google/mobly/blob/c287393948cb53d2fedfcfd062092705578f593e/mobly/controllers/android_device.py#L408)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Is it set_debug_tag? Or just debug_tag('caller')?
</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->

adorokhine: 



Reviewed 1 of 2 files at r4, 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

*[mobly/controllers/android_device.py, line 53 at r4](https://reviewable.io:443/reviews/google/mobly/107#-KeeYOfb4WvMXMwaepwT:-Kf9MrMjYIpNn_oii9kS:b-1pjh3z) ([raw file](https://github.com/google/mobly/blob/c287393948cb53d2fedfcfd062092705578f593e/mobly/controllers/android_device.py#L53)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

I specifically want this error class so the msg is easier to read.
The default repr msg is quite ugly since it's just a tuple of reprs.
I want msg from AndroidDevice object to be cleaner than that.

Also this error serves as a distinction between errors from `android_device` module and `AndroidDevice` class.
</blockquote></details>

Sure, if it's just a regular exception not meant for introspective/dynamic manipulation can we drop the generic *args and just make it a regular exception with `__init__(self, ad, msg)`, and drop the massaging of the args tuple?

---

*[mobly/controllers/android_device.py, line 92 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9N9ojBOB2HUR5U8_C:-Kf9N9ojBOB2HUR5U8_D:b-9v83il) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/mobly/controllers/android_device.py#L92)):*
> ```Python
>     for ad in ads:
>         if ad.serial not in connected_ads:
>             raise ad._error('Android device is specified in config but is not '
> ```

Does ad._error still exist? Directly raising the exception is preferred from my side.

---

*[mobly/controllers/android_device.py, line 383 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9O1dtGo40qBoVR8If:-Kf9O1duz07yq2S8swhh:b-5xklzx) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/mobly/controllers/android_device.py#L383)):*
> ```Python
> 
>     def __repr__(self):
>         return "<AndroidDevice|%s>" % self.debug_tag
> ```

Copy of LOG_TAG_TEMPLATE but with different choice of brackets? We should just reuse LOG_TAG_TEMPLATE for this.

---

*[mobly/controllers/android_device.py, line 686 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9OPNaZO5YL36F6EF4:-Kf9OPNb9iEkdO2Eecin:b9i1bsi) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/mobly/controllers/android_device.py#L686)):*
> ```Python
>         """
>         if not self.adb_logcat_file_path:
>             raise DeviceError(self, 
> ```

Trailing whitespace, here and elsewhere. Is this what yapf produced?

---

*[mobly/controllers/android_device.py, line 900 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9Ok5GnXdu4dmnRKAE:-Kf9Ok5GnXdu4dmnRKAF:b49kzb9) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/mobly/controllers/android_device.py#L900)):*
> ```Python
> 
>     def process(self, msg, kwargs):
>         msg = _LOG_TAG_TEMPLATE % (self.extra['tag'], msg)
> ```

Would it be wise to stop distinguishing between the prefix and tag and just let the user supply the whole thing? If someone extended AndroidDevice they'd be unable to change the class name currently. Plus the documentation is confusing this way because tag and prefix and related but not quite the same.

---

*[mobly/controllers/android_device_lib/jsonrpc_client_base.py, line 191 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9P8mHjksbul80CbWP:-Kf9P8mHjksbul80CbWQ:b-26gels) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/mobly/controllers/android_device_lib/jsonrpc_client_base.py#L191)):*
> ```Python
>         raise AppStartError('%s failed to start on %s.' %
>                             (self.app_name, self._adb.serial))
>         self.log.debug('Successfully started %s', self.app_name)
> ```

This will never execute

---

*[tests/mobly/controllers/android_device_test.py, line 421 at r5](https://reviewable.io:443/reviews/google/mobly/107#-Kf9PGC6BdGrKQymsST_:-Kf9PGC6BdGrKQymsSTa:bqugfe1) ([raw file](https://github.com/google/mobly/blob/286b1966226f81b00ffb562cb660f6ef8254877d/tests/mobly/controllers/android_device_test.py#L421)):*
> ```Python
>         self.assertFalse(hasattr(ad, 'snippet'))
> 
>     @mock.patch('mobly.controllers.android_device_lib.adb.AdbProxy', return_value=mock_android_device.MockAdbProxy(1))
> ```

Has this been through yapf?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/107)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index afe5a79..79d3f16 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -35,6 +35,7 @@ from mobly.controllers.android_device_lib import snippet_client
 MOBLY_CONTROLLER_CONFIG_NAME = 'AndroidDevice'
 
 ANDROID_DEVICE_PICK_ALL_TOKEN = '*'
+_DEBUG_PREFIX_TEMPLATE = '[AndroidDevice|%s] %s'
 
 # Key name for adb logcat extra params in config file.
 ANDROID_DEVICE_ADB_LOGCAT_PARAM_KEY = 'adb_logcat_param'
@@ -49,12 +50,16 @@ class Error(signals.ControllerError):
     pass
 
 
-class SnippetError(signals.ControllerError):
-    """Raised when somethig wrong with snippet happens."""
+class DeviceError(Error):
+    """Raised for errors from within an AndroidDevice object."""
 
+    def __init__(self, ad, msg):
+        new_msg = '%s %s' % (repr(ad), msg)
+        super(DeviceError, self).__init__(new_msg)
 
-class DoesNotExistError(Error):
-    """Raised when something that does not exist is referenced."""
+
+class SnippetError(DeviceError):
+    """Raised for errors related to Mobly snippet."""
 
 
 def create(configs):
@@ -83,8 +88,8 @@ def create(configs):
 
     for ad in ads:
         if ad.serial not in connected_ads:
-            raise DoesNotExistError('Android device %s is specified in config'
-                                    ' but is not attached.' % ad.serial)
+            raise DeviceError(ad, 'Android device is specified in config but '
+                              ' is not attached.')
     _start_services_on_ads(ads)
     return ads
 
@@ -133,15 +138,15 @@ def _start_services_on_ads(ads):
         running_ads.append(ad)
         try:
             ad.start_services()
-        except Exception as e:
+        except Exception:
             is_required = getattr(ad, KEY_DEVICE_REQUIRED, True)
             if is_required:
                 ad.log.exception('Failed to start some services, abort!')
                 destroy(running_ads)
                 raise
             else:
-                logging.warning('Skipping device %s because some service '
-                                'failed to start: %s', ad.serial, e)
+                ad.log.exception('Skipping this optional device because some '
+                                 'services failed to start.')
 
 
 def _parse_device_list(device_list_str, key):
@@ -225,10 +230,10 @@ def get_instances_with_configs(configs):
         try:
             ad = AndroidDevice(serial)
             ad.load_config(c)
-        except Exception as e:
+        except Exception:
             if is_required:
                 raise
-            logging.warning('Skipping device %s due to error: %s', serial, e)
+            ad.log.exception('Skipping this optional device due to error.')
             continue
         results.append(ad)
     return results
@@ -344,8 +349,8 @@ class AndroidDevice(object):
                   android device should be stored.
         log: A logger adapted from root logger with an added prefix specific
              to an AndroidDevice instance. The default prefix is
-             [AndroidDevice|<serial>]. Use self.set_logger_prefix_tag to use
-             a different tag in the prefix.
+             [AndroidDevice|<serial>]. Use self.set_debug_tag to use a
+             different tag in the prefix.
         adb_logcat_file_path: A string that's the full path to the adb logcat
                               file collected, if any.
         adb: An AdbProxy object used for interacting with the device via adb.
@@ -358,8 +363,9 @@ class AndroidDevice(object):
         # logging.log_path only exists when this is used in an Mobly test run.
         log_path_base = getattr(logging, 'log_path', '/tmp/logs')
         self.log_path = os.path.join(log_path_base, 'AndroidDevice%s' % serial)
+        self._debug_tag = self.serial
         self.log = AndroidDeviceLoggerAdapter(logging.getLogger(),
-                                              {'tag': self.serial})
+                                              {'tag': self.debug_tag})
         self.sl4a = None
         self.ed = None
         self._adb_logcat_process = None
@@ -372,22 +378,36 @@ class AndroidDevice(object):
         # names, values are the clients: {<attr name string>: <client object>}.
         self._snippet_clients = {}
 
-    def set_logger_prefix_tag(self, tag):
-        """Set a tag for the log line prefix of this instance.
+    def __repr__(self):
+        return "<AndroidDevice|%s>" % self.debug_tag
 
-        By default, the tag is the serial of the device, but sometimes having
-        the serial number in the log line doesn't help much with debugging. It
-        could be more helpful if users can mark the role of the device instead.
+    @property
+    def debug_tag(self):
+        """A string that represents a device object in debug info. Default value
+        is the device serial.
 
-        For example, instead of marking the serial number:
-            'INFO [AndroidDevice|abcdefg12345] One pending call ringing.'
+        This will be used as part of the prefix of debugging messages emitted by
+        this device object, like log lines and the message of DeviceError.
+        """
+        return self._debug_tag
 
-        marking the role of the device here is  more useful here:
-            'INFO [AndroidDevice|Caller] One pending call ringing.'
+    @debug_tag.setter
+    def debug_tag(self, tag):
+        """Setter for the debug tag.
 
-        Args:
-            tag: A string that is the tag to use.
+        By default, the tag is the serial of the device, but sometimes it may
+        be more descriptive to use a different tag of the user's choice.
+
+        Changing debug tag changes part of the prefix of debug info emitted by
+        this object, like log lines and the message of DeviceError.
+
+        Example:
+            By default, the device's serial number is used:
+                'INFO [AndroidDevice|abcdefg12345] One pending call ringing.'
+            The tag can be customized with `ad.debug_tag = 'Caller'`:
+                'INFO [AndroidDevice|Caller] One pending call ringing.'
         """
+        self._debug_tag = tag
         self.log.extra['tag'] = tag
 
     def start_services(self):
@@ -542,8 +562,9 @@ class AndroidDevice(object):
         """
         for k, v in config.items():
             if hasattr(self, k):
-                raise Error('Attempting to set existing attribute %s on %s' %
-                            (k, self.serial))
+                raise DeviceError(self, (
+                    'Attribute %s already exists with value %s, cannot set '
+                    'again.') % (k, getattr(self, k)))
             setattr(self, k, v)
 
     def root_adb(self):
@@ -576,17 +597,21 @@ class AndroidDevice(object):
         # Should not load snippet with the same attribute more than once.
         if name in self._snippet_clients:
             raise SnippetError(
+                self,
                 'Attribute "%s" is already registered with package "%s", it '
                 'cannot be used again.' %
                 (name, self._snippet_clients[name].package))
         # Should not load snippet with an existing attribute.
         if hasattr(self, name):
-            raise SnippetError('Attribute "%s" already exists, please use a '
-                               'different name.' % name)
+            raise SnippetError(
+                self,
+                'Attribute "%s" already exists, please use a different name.' %
+                name)
         # Should not load the same snippet package more than once.
         for client_name, client in self._snippet_clients.items():
             if package == client.package:
                 raise SnippetError(
+                    self,
                     'Snippet package "%s" has already been loaded under name'
                     ' "%s".' % (package, client_name))
         host_port = utils.get_available_host_port()
@@ -660,9 +685,9 @@ class AndroidDevice(object):
                 period.
         """
         if not self.adb_logcat_file_path:
-            raise Error(
-                'Attempting to cat adb log when none has been collected on '
-                'Android device %s.' % self.serial)
+            raise DeviceError(
+                self,
+                'Attempting to cat adb log when none has been collected.')
         end_time = mobly_logger.get_log_line_timestamp()
         self.log.debug('Extracting adb log from logcat.')
         adb_excerpt_path = os.path.join(self.log_path, 'AdbLogExcerpts')
@@ -704,8 +729,9 @@ class AndroidDevice(object):
         save the logcat in a file.
         """
         if self._adb_logcat_process:
-            raise Error('Android device %s already has an adb logcat thread '
-                        'going on. Cannot start another one.' % self.serial)
+            raise DeviceError(
+                self,
+                'Logcat thread is already running, cannot start another one.')
         # Disable adb log spam filter for rootable. Have to stop and clear
         # settings first because 'start' doesn't support --clear option before
         # Android N.
@@ -728,9 +754,7 @@ class AndroidDevice(object):
         """Stops the adb logcat collection subprocess.
         """
         if not self._adb_logcat_process:
-            raise Error(
-                'Android device %s does not have an ongoing adb logcat '
-                'collection.' % self.serial)
+            raise DeviceError(self, 'No ongoing adb logcat collection found.')
         utils.stop_standing_subprocess(self._adb_logcat_process)
         self._adb_logcat_process = None
 
@@ -764,8 +788,7 @@ class AndroidDevice(object):
         if new_br:
             out = self.adb.shell('bugreportz').decode('utf-8')
             if not out.startswith('OK'):
-                raise Error('Failed to take bugreport on %s: %s' %
-                            (self.serial, out))
+                raise DeviceError(self, 'Failed to take bugreport: %s' % out)
             br_out_path = out.split(':')[1].strip()
             self.adb.pull('%s %s' % (br_out_path, full_out_path))
         else:
@@ -826,7 +849,7 @@ class AndroidDevice(object):
                 # process, which is normal. Ignoring these errors.
                 pass
             time.sleep(5)
-        raise Error('Device %s booting process timed out.' % self.serial)
+        raise DeviceError(self, 'Booting process timed out.')
 
     def _get_active_snippet_info(self):
         """Collects information on currently active snippet clients.
@@ -878,5 +901,5 @@ class AndroidDeviceLoggerAdapter(logging.LoggerAdapter):
     """
 
     def process(self, msg, kwargs):
-        msg = '[AndroidDevice|%s] %s' % (self.extra['tag'], msg)
+        msg = _DEBUG_PREFIX_TEMPLATE % (self.extra['tag'], msg)
         return (msg, kwargs)
diff --git a/mobly/controllers/android_device_lib/callback_handler.py b/mobly/controllers/android_device_lib/callback_handler.py
index 7c648c2..9df09c0 100644
--- a/mobly/controllers/android_device_lib/callback_handler.py
+++ b/mobly/controllers/android_device_lib/callback_handler.py
@@ -18,6 +18,12 @@ import time
 
 from mobly.controllers.android_device_lib import snippet_event
 
+# The max timeout cannot be larger than the max time the socket waits for a
+# response message. Otherwise, the socket would timeout before the Rpc call
+# does, leaving both server and client in unknown states.
+MAX_TIMEOUT = 60 * 10
+DEFAULT_TIMEOUT = 120  # two minutes
+
 
 class Error(Exception):
     pass
@@ -56,7 +62,7 @@ class CallbackHandler(object):
         self.ret_value = ret_value
         self._method_name = method_name
 
-    def waitAndGet(self, event_name, timeout=None):
+    def waitAndGet(self, event_name, timeout=DEFAULT_TIMEOUT):
         """Blocks until an event of the specified name has been received and
         return the event, or timeout.
 
@@ -68,9 +74,15 @@ class CallbackHandler(object):
             SnippetEvent, the oldest entry of the specified event.
 
         Raises:
+            Error: If the specified timeout is longer than the max timeout
+                   supported.
             TimeoutError: The expected event does not occur within time limit.
         """
         if timeout:
+            if timeout > MAX_TIMEOUT:
+                raise Error(
+                    'Specified timeout %s is longer than max timeout %s.' %
+                    (timeout, MAX_TIMEOUT))
             timeout *= 1000  # convert to milliseconds for java side
         try:
             raw_event = self._event_client.eventWaitAndGet(self._id,
@@ -78,11 +90,56 @@ class CallbackHandler(object):
         except Exception as e:
             if 'EventSnippetException: timeout.' in str(e):
                 raise TimeoutError(
-                    'Timeout waiting for event "%s" triggered by %s (%s).'
-                    % (event_name, self._method_name, self._id))
+                    'Timeout waiting for event "%s" triggered by %s (%s).' %
+                    (event_name, self._method_name, self._id))
             raise
         return snippet_event.from_dict(raw_event)
 
+    def waitForEvent(self, event_name, predicate, timeout=DEFAULT_TIMEOUT):
+        """Wait for an event of a specific name that satisfies the predicate.
+
+        This call will block until the expected event has been received or time
+        out.
+
+        The predicate function defines the condition the event is expected to
+        satisfy. It takes an event and returns True if the condition is
+        satisfied, False otherwise.
+
+        Note all events of the same name that are received but don't satisfy
+        the predicate will be discarded and not be available for further
+        consumption.
+
+        Args:
+          event_name: string, the name of the event to wait for.
+          predicate: function, a function that takes an event (dictionary) and
+                     returns a bool.
+          timeout: float, default is 120s.
+
+        Returns:
+          dictionary, the event that satisfies the predicate if received.
+
+        Raises:
+          TimeoutError: raised if no event that satisfies the predicate is
+                        received after timeout seconds.
+        """
+        deadline = time.time() + timeout
+        while time.time() <= deadline:
+            # Calculate the max timeout for the next event rpc call.
+            rpc_timeout = deadline - time.time()
+            if rpc_timeout < 0:
+                break
+            try:
+                event = self.waitAndGet(event_name, rpc_timeout)
+            except TimeoutError:
+                # Ignoring TimeoutError since we need to throw one with a more
+                # specific message.
+                break
+            if predicate(event):
+                return event
+        raise TimeoutError(
+            'Timed out after %ss waiting for an "%s" event that satisfies the '
+            'predicate "%s".' % (timeout, event_name, predicate.__name__))
+
     def getAll(self, event_name):
         """Gets all the events of a certain name that have been received so
         far. This is a non-blocking call.
diff --git a/mobly/controllers/android_device_lib/jsonrpc_client_base.py b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
index bb4b5ed..187ec1c 100644
--- a/mobly/controllers/android_device_lib/jsonrpc_client_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
@@ -39,6 +39,7 @@ Response:
 from builtins import str
 
 import json
+import logging
 import socket
 import threading
 import time
@@ -53,7 +54,10 @@ APP_START_WAIT_TIME = 15
 UNKNOWN_UID = -1
 
 # Maximum time to wait for the socket to open on the device.
-_SOCKET_TIMEOUT = 60
+_SOCKET_CONNECTION_TIMEOUT = 60
+
+# Maximum time to wait for a response message on the socket.
+_SOCKET_READ_TIMEOUT = callback_handler.MAX_TIMEOUT
 
 
 class Error(Exception):
@@ -70,9 +74,9 @@ class ApiError(Error):
 
 class ProtocolError(Error):
     """Raised when there is some error in exchanging data with server."""
-    NO_RESPONSE_FROM_HANDSHAKE = "No response from handshake."
-    NO_RESPONSE_FROM_SERVER = "No response from server."
-    MISMATCHED_API_ID = "Mismatched API id."
+    NO_RESPONSE_FROM_HANDSHAKE = 'No response from handshake.'
+    NO_RESPONSE_FROM_SERVER = 'No response from server.'
+    MISMATCHED_API_ID = 'Mismatched API id.'
 
 
 class JsonRpcCommand(object):
@@ -102,7 +106,12 @@ class JsonRpcClientBase(object):
         uid: (int) The uid of this session.
     """
 
-    def __init__(self, host_port, device_port, app_name, adb_proxy):
+    def __init__(self,
+                 host_port,
+                 device_port,
+                 app_name,
+                 adb_proxy,
+                 log=logging.getLogger()):
         """
         Args:
             host_port: (int) The host port of this RPC client.
@@ -121,6 +130,7 @@ class JsonRpcClientBase(object):
         self._counter = None
         self._lock = threading.Lock()
         self._event_client = None
+        self._log = log
 
     def __del__(self):
         self.close()
@@ -178,6 +188,7 @@ class JsonRpcClientBase(object):
         for _ in range(wait_time):
             time.sleep(1)
             if self._is_app_running():
+                self.log.debug('Successfully started %s', self.app_name)
                 return
         raise AppStartError('%s failed to start on %s.' %
                             (self.app_name, self._adb.serial))
@@ -186,9 +197,9 @@ class JsonRpcClientBase(object):
         """Opens a connection to a JSON RPC server.
 
         Opens a connection to a remote client. The connection attempt will time
-        out if it takes longer than _SOCKET_TIMEOUT seconds. Each subsequent
-        operation over this socket will time out after _SOCKET_TIMEOUT seconds
-        as well.
+        out if it takes longer than _SOCKET_CONNECTION_TIMEOUT seconds. Each
+        subsequent operation over this socket will time out after
+        _SOCKET_READ_TIMEOUT seconds as well.
 
         Args:
             uid: int, The uid of the session to join, or UNKNOWN_UID to start a
@@ -202,8 +213,8 @@ class JsonRpcClientBase(object):
         """
         self._counter = self._id_counter()
         self._conn = socket.create_connection(('127.0.0.1', self.host_port),
-                                              _SOCKET_TIMEOUT)
-        self._conn.settimeout(_SOCKET_TIMEOUT)
+                                              _SOCKET_CONNECTION_TIMEOUT)
+        self._conn.settimeout(_SOCKET_READ_TIMEOUT)
         self._client = self._conn.makefile(mode='brw')
 
         resp = self._cmd(cmd, uid)
diff --git a/mobly/controllers/android_device_lib/sl4a_client.py b/mobly/controllers/android_device_lib/sl4a_client.py
index ef5583e..ba31e0f 100644
--- a/mobly/controllers/android_device_lib/sl4a_client.py
+++ b/mobly/controllers/android_device_lib/sl4a_client.py
@@ -57,4 +57,4 @@ class Sl4aClient(jsonrpc_client_base.JsonRpcClientBase):
             "pm list package | grep com.googlecode.android_scripting"):
             raise jsonrpc_client_base.AppStartError(
                 '%s is not installed on %s' % (
-                self.app_name, self._adb.getprop('ro.boot.serialno')))
+                self.app_name, self._adb.serial))
diff --git a/mobly/controllers/android_device_lib/snippet_client.py b/mobly/controllers/android_device_lib/snippet_client.py
index 7c3ca01..f3ffade 100644
--- a/mobly/controllers/android_device_lib/snippet_client.py
+++ b/mobly/controllers/android_device_lib/snippet_client.py
@@ -54,10 +54,11 @@ class SnippetClient(jsonrpc_client_base.JsonRpcClientBase):
             host_port=host_port,
             device_port=host_port,
             app_name=package,
-            adb_proxy=adb_proxy)
+            adb_proxy=adb_proxy,
+            log=log)
         self.package = package
-        self._serial = self._adb.getprop('ro.boot.serialno')
         self.log = log
+        self._serial = self._adb.serial
 
     def _do_start_app(self):
         """Overrides superclass."""


Test patch:
diff --git a/tests/mobly/controllers/android_device_lib/callback_handler_test.py b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
index d121fe8..c3493f9 100755
--- a/tests/mobly/controllers/android_device_lib/callback_handler_test.py
+++ b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
@@ -37,6 +37,10 @@ class CallbackHandlerTest(unittest.TestCase):
     """Unit tests for mobly.controllers.android_device_lib.callback_handler.
     """
 
+    def test_timeout_value(self):
+        self.assertGreaterEqual(jsonrpc_client_base._SOCKET_READ_TIMEOUT,
+                                callback_handler.MAX_TIMEOUT)
+
     def test_event_dict_to_snippet_event(self):
         mock_event_client = mock.Mock()
         mock_event_client.eventWaitAndGet = mock.Mock(
@@ -54,7 +58,8 @@ class CallbackHandlerTest(unittest.TestCase):
 
     def test_wait_and_get_timeout(self):
         mock_event_client = mock.Mock()
-        java_timeout_msg = 'com.google.android.mobly.snippet.event.EventSnippet$EventSnippetException: timeout.'
+        java_timeout_msg = ('com.google.android.mobly.snippet.event.'
+                            'EventSnippet$EventSnippetException: timeout.')
         mock_event_client.eventWaitAndGet = mock.Mock(
             side_effect=jsonrpc_client_base.ApiError(java_timeout_msg))
         handler = callback_handler.CallbackHandler(
@@ -67,6 +72,41 @@ class CallbackHandlerTest(unittest.TestCase):
                                      expected_msg):
             handler.waitAndGet('ha')
 
+    def test_wait_for_event(self):
+        mock_event_client = mock.Mock()
+        mock_event_client.eventWaitAndGet = mock.Mock(
+            return_value=MOCK_RAW_EVENT)
+        handler = callback_handler.CallbackHandler(
+            callback_id=MOCK_CALLBACK_ID,
+            event_client=mock_event_client,
+            ret_value=None,
+            method_name=None)
+
+        def some_condition(event):
+            return event.data['successful']
+
+        event = handler.waitForEvent('AsyncTaskResult', some_condition, 0.01)
+
+    def test_wait_for_event_negative(self):
+        mock_event_client = mock.Mock()
+        mock_event_client.eventWaitAndGet = mock.Mock(
+            return_value=MOCK_RAW_EVENT)
+        handler = callback_handler.CallbackHandler(
+            callback_id=MOCK_CALLBACK_ID,
+            event_client=mock_event_client,
+            ret_value=None,
+            method_name=None)
+        expected_msg = (
+            'Timed out after 0.01s waiting for an "AsyncTaskResult" event that'
+            ' satisfies the predicate "some_condition".')
+
+        def some_condition(event):
+            return False
+
+        with self.assertRaisesRegexp(callback_handler.TimeoutError,
+                                     expected_msg):
+            handler.waitForEvent('AsyncTaskResult', some_condition, 0.01)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index d66b4dc..38247cb 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -206,7 +206,7 @@ class AndroidDeviceTest(unittest.TestCase):
         """
         mock_serial = 1
         ad = android_device.AndroidDevice(serial=mock_serial)
-        expected_msg = "Failed to take bugreport on 1: OMG I died!"
+        expected_msg = ".* Failed to take bugreport."
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             ad.take_bug_report("test_something", "sometime")
@@ -244,8 +244,7 @@ class AndroidDeviceTest(unittest.TestCase):
         """
         mock_serial = 1
         ad = android_device.AndroidDevice(serial=mock_serial)
-        expected_msg = ("Android device .* does not have an ongoing adb logcat"
-                        " collection.")
+        expected_msg = ".* No ongoing adb logcat collection found."
         # Expect error if stop is called before start.
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
@@ -261,8 +260,8 @@ class AndroidDeviceTest(unittest.TestCase):
         start_proc_mock.assert_called_with(adb_cmd % (ad.serial,
                                                       expected_log_path))
         self.assertEqual(ad.adb_logcat_file_path, expected_log_path)
-        expected_msg = ("Android device .* already has an adb logcat thread "
-                        "going on. Cannot start another one.")
+        expected_msg = ('Logcat thread is already running, cannot start another'
+                        ' one.')
         # Expect error if start is called back to back.
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
@@ -289,8 +288,7 @@ class AndroidDeviceTest(unittest.TestCase):
         mock_serial = 1
         ad = android_device.AndroidDevice(serial=mock_serial)
         ad.adb_logcat_param = "-b radio"
-        expected_msg = ("Android device .* does not have an ongoing adb logcat"
-                        " collection.")
+        expected_msg = '.* No ongoing adb logcat collection found.'
         # Expect error if stop is called before start.
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
@@ -324,8 +322,8 @@ class AndroidDeviceTest(unittest.TestCase):
         mock_serial = 1
         ad = android_device.AndroidDevice(serial=mock_serial)
         # Expect error if attempted to cat adb log before starting adb logcat.
-        expected_msg = ("Attempting to cat adb log when none has been "
-                        "collected on Android device .*")
+        expected_msg = (".* Attempting to cat adb log when none"
+                        " has been collected.")
         with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             ad.cat_adb_log("some_test", MOCK_ADB_LOGCAT_BEGIN_TIME)
@@ -370,7 +368,7 @@ class AndroidDeviceTest(unittest.TestCase):
         ad.load_snippet('snippet', MOCK_SNIPPET_PACKAGE_NAME)
         expected_msg = ('Snippet package "%s" has already been loaded under '
                         'name "snippet".') % MOCK_SNIPPET_PACKAGE_NAME
-        with self.assertRaisesRegexp(android_device.SnippetError,
+        with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             ad.load_snippet('snippet2', MOCK_SNIPPET_PACKAGE_NAME)
 
@@ -388,7 +386,7 @@ class AndroidDeviceTest(unittest.TestCase):
         expected_msg = ('Attribute "%s" is already registered with package '
                         '"%s", it cannot be used again.') % (
                         'snippet', MOCK_SNIPPET_PACKAGE_NAME)
-        with self.assertRaisesRegexp(android_device.SnippetError,
+        with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             ad.load_snippet('snippet', MOCK_SNIPPET_PACKAGE_NAME + 'haha')
 
@@ -403,7 +401,7 @@ class AndroidDeviceTest(unittest.TestCase):
         ad = android_device.AndroidDevice(serial=1)
         expected_msg = ('Attribute "%s" already exists, please use a different'
                         ' name') % 'adb'
-        with self.assertRaisesRegexp(android_device.SnippetError,
+        with self.assertRaisesRegexp(android_device.Error,
                                      expected_msg):
             ad.load_snippet('adb', MOCK_SNIPPET_PACKAGE_NAME)
 
@@ -420,6 +418,30 @@ class AndroidDeviceTest(unittest.TestCase):
         ad.stop_services()
         self.assertFalse(hasattr(ad, 'snippet'))
 
+    @mock.patch('mobly.controllers.android_device_lib.adb.AdbProxy',
+                return_value=mock_android_device.MockAdbProxy(1))
+    @mock.patch('mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+                return_value=mock_android_device.MockFastbootProxy(1))
+    def test_AndroidDevice_debug_tag(self, MockFastboot, MockAdbProxy):
+        mock_serial = 1
+        ad = android_device.AndroidDevice(serial=mock_serial)
+        self.assertEqual(ad.debug_tag, 1)
+        try:
+            raise android_device.DeviceError(ad, 'Something')
+        except android_device.DeviceError as e:
+            self.assertEqual('<AndroidDevice|1> Something', str(e))
+        # Verify that debug tag's setter updates the debug prefix correctly.
+        ad.debug_tag = 'Mememe'
+        try:
+            raise android_device.DeviceError(ad, 'Something')
+        except android_device.DeviceError as e:
+            self.assertEqual('<AndroidDevice|Mememe> Something', str(e))
+        # Verify that repr is changed correctly.
+        try:
+            raise Exception(ad, 'Something')
+        except Exception as e:
+            self.assertEqual("(<AndroidDevice|Mememe>, 'Something')", str(e))
+
 
 if __name__ == "__main__":
     unittest.main()