refactor(prov-dev, deps): Add better logging to provisioning device client #746
Conversation
|
/azp run Java Prod |
|
Azure Pipelines successfully started running 1 pipeline(s). |
timtay-microsoft
force-pushed
the
timtay/provisioningLogging
branch
from
2b05f0d to
ff2bae9
Compare
|
/azp run Java Prod, SDL, horton-java-gate |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| DeliveryState remoteState = d.getRemoteState(); | ||
|
|
||
| // Codes_SRS_AMQPSIOTHUBCONNECTION_15_039: [The event handler shall note the remote delivery state and use it and the Delivery (Proton) hash code to inform the AmqpsIotHubConnection of the message receipt.] | ||
| boolean messageAcknowledgedAsSuccess = remoteState.equals(Accepted.getInstance()); |
There was a problem hiding this comment.
Previously, we ignored if our sent message was accepted or not. Now we fire a callback up to the listener to notify it if any message that was sent was rejected/abandoned/released/etc by the service so that it can stop waiting and propagate up the exception to the user
| @@ -18,38 +18,30 @@ public static void verifyHttpResponse(HttpResponse httpResponse) throws Provisio | |||
| { | |||
| int responseStatus = httpResponse.getStatus(); | |||
|
|
|||
| String errorMessage = ErrorMessageParser.bestErrorMessage(new String(httpResponse.getErrorReason(), StandardCharsets.UTF_8)); | |||
There was a problem hiding this comment.
There is no reason for us to parse the error message considering it will just be a string to the exception we give it to. Parsing this error message may even filter out important information from the service, so we're better off just giving the full error payload string to the user
| String errorMessage = ErrorMessageParser.bestErrorMessage(new String(httpResponse.getErrorReason(), StandardCharsets.UTF_8)); | ||
| byte[] errorReason = httpResponse.getErrorReason(); | ||
|
|
||
| String errorMessage = httpResponse.getStatus() + " : " + new String(errorReason, StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Previously, we didn't include the status code in this error message, but we should as it helps debug customer issues
| case 502: | ||
| throw new ProvisioningDeviceHubException(errorMessage); | ||
| case 503: |
There was a problem hiding this comment.
This was a strange switch case where almost all cases did the exact same thing. Just using a fall through to simplify it a bit
|
/azp run Java Prod Basic |
|
Azure Pipelines successfully started running 1 pipeline(s). |
deps/src/main/java/com/microsoft/azure/sdk/iot/deps/transport/amqp/AmqpsConnection.java
Show resolved
Hide resolved
| @@ -139,6 +130,11 @@ public boolean isConnected() throws Exception | |||
| throw this.saslListener.getSavedException(); | |||
| } | |||
|
|
|||
| if (this.protonJExceptionParser != null && this.protonJExceptionParser.getError() != null) | |||
There was a problem hiding this comment.
Wouldn't onConnectionInit() be a better place for this check?
There was a problem hiding this comment.
isConnected() is called from a user thread whereas onConnectionInit() is a reactor thread. We only want to throw exceptions from a user thread, so I'd prefer to keep this saved exception check here.
There was a problem hiding this comment.
I see, maybe add a comment to that effect?
deps/src/main/java/com/microsoft/azure/sdk/iot/deps/transport/amqp/AmqpsConnection.java
Outdated
Show resolved
Hide resolved
deps/src/main/java/com/microsoft/azure/sdk/iot/deps/transport/amqp/AmqpsConnection.java
Outdated
Show resolved
Hide resolved
deps/src/main/java/com/microsoft/azure/sdk/iot/deps/transport/amqp/AmqpsConnection.java
Outdated
Show resolved
Hide resolved
...oft/azure/sdk/iot/provisioning/device/internal/contract/amqp/ProvisioningAmqpOperations.java
Show resolved
Hide resolved
.../com/microsoft/azure/sdk/iot/provisioning/device/internal/contract/http/ContractAPIHttp.java
Show resolved
Hide resolved
…lient Also fixing an issue where provisioning device client didn't propagate up the error code if an http exception was returned from service Also fixing a bug where if the provisioning device client ignored amqp messages getting rejected from service. Now the sdk responds and throws appropriately
timtay-microsoft
force-pushed
the
timtay/provisioningLogging
branch
from
f63dbbb to
0fc4c8b
Compare
|
/azp run Java Prod, SDL, horton-java-gate |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run Java Prod, SDL, horton-java-gate |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run Java Prod Basic |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Also fixing an issue where provisioning device client didn't propagate up the error code if an http exception was returned from service
Also fixing a bug where if the provisioning device client ignored amqp messages getting rejected from service. Now the sdk responds and throws appropriately