WPJ related error codes #2486
WPJ related error codes #2486
Conversation
melissaahn
added
the
No-Changelog
| /** | ||
| * An error occurred related to device registration. | ||
| */ | ||
| public static final String WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR = "workplace_join_device_registration_error"; | ||
|
|
||
| /** | ||
| * An error occurred related to device attribute patching. | ||
| */ | ||
| public static final String WORKPLACE_JOIN_DEVICE_ATTRIBUTE_PATCHING_ERROR = "workplace_join_device_attribute_patching_error"; | ||
|
|
||
| /** | ||
| * An error occurred related to getting device state. | ||
| */ | ||
| public static final String WORKPLACE_JOIN_DEVICE_STATE_ERROR = "workplace_join_device_state_error"; |
There was a problem hiding this comment.
These seem to generic. They just seem to indicate that something went wrong but don't tell exactly what went wrong. Will these be actionable?
There was a problem hiding this comment.
One of these (WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR) is involved with the direct catching of a WorkplaceJoinException which was in a sort of catch-all block. Further figuring out why this WorkplaceJoinException is being thrown is possible if there's already a throwable attached to it (we could maybe run the ExceptionAdapter on it?), so let me know if that's preferable....
Other cases involve replacing instances where catch blocks create ClientException with UNKNOWN_ERROR directly for unmodeled errors. My understanding of the intention of this was to not lose the location where the original exception gets caught (I think in telemetry, the "location" is where the ClientException got created, and the cause location is where the throwable exception got created). By using the ExceptionAdapter method, I think that "location" value becomes somewhat useless, since it'll lead back to ExceptionAdapter.
I was thinking that since we were considering turning WPJExceptions into a BaseException, which means WPJ might be a big enough feature to warrant throwing the exception directly, it might be helpful to keep that location information. I could be wrong though.
I also understand that it could be weird to have instances of ClientExceptions with a null pointer error code, and others with a wpj error code but a NPE attached.
There was a problem hiding this comment.
In general, it would be good to be able to distinguish one particular WORKPLACE_JOIN_DEVICE_REGISTRATION_ERROR vs another. It can be done by either having a different location/cause or a different error code altogether. I think if there are very few instances then cause will suffice but if there are lots of different instances then we may consider having dedicated errors codes to track these. I think for now what you've done is fine and we should iterate again based on learnings we get from telemetry once this ships.
Summary
This PR creates new error codes to help reclassify some ClientExceptions thrown in WPJ related code: https://github.com/AzureAD/ad-accounts-for-android/pull/2921