[core-amqp] MessagingError changes #6673

chradek · 2019-12-21T00:30:21Z

There's one test that's failing still (the amqp error test), but otherwise everything else is green. Let's use this as a starting point for the linked issue.

/cc @ramya-rao-a @richardpark-msft

ramya-rao-a · 2020-01-02T03:11:18Z

sdk/core/core-amqp/src/errors.ts

    }
-    if (retryableErrors.indexOf(error.name) === -1) {
+    if (error.code && retryableErrors.indexOf(error.code) === -1) {


What if by this point we don't have a code?

At this point I was treating errors without a code as neither an AmqpError nor a SystemError, so do not alter them. They currently don't have a retryable field so are effectively not retryable.

This could change based on the thread you started at https://github.com/Azure/azure-sdk-for-js/pull/6673/files#r362364248

While isAmqpError() ensures that the error has the condition property, we still won't have a code if there was an error with an amqp condition for which we have no mapping. Such errors will be treated as retryable.

Currently, if there isn't a code, we will treat the error as retryable since retryable is set to true in the MessagingError constructor and we don't change it to false here. That seems consistent with the behavior prior to this PR, since if no condition was found we'd set the name to MessagingError, and any error with the name MessagingError was treated as retryable.

So, I think the question you're getting at is whether we should set error.code if there is no mapping or not. My preference would be to not do this, because then it would be a breaking change if we wanted to change the code in the future for an error we learned more about but weren't handling before. I think we can safely add a code if it never existed in the 1st place, but not change it if there already was one for an error.

That seems consistent with the behavior prior to this PR, since if no condition was found we'd set the name to MessagingError, and any error with the name MessagingError was treated as retryable.

Not true... retryableErrors would not have an entry for MessagingError and so retryable would be set to false...

azure-sdk-for-js/sdk/core/core-amqp/src/errors.ts

Line 490 in 4b0f3ad

"MessagingError",

retryableErrors does have an entry for MessagingError today though...

ah! I missed that :)

sdk/core/core-amqp/src/errors.ts

ramya-rao-a · 2020-01-02T03:30:06Z

sdk/core/core-amqp/src/errors.ts

@@ -464,9 +468,23 @@ export class MessagingError extends Error {
  info?: any;
  /**
   * @param {string} message The error message that provides more information about the error.
+   * @param originalError An error whose properties will be copied to the MessagingError if the
+   * propert doesn't already exist.


Why copy only if the property already exists and not copy all? They will get overriden appropriately after the constructor call right?

Since the properties are copied after the super call, we don't want to end up overwriting the message, name, and stack properties with those from the original error. This also means if the error had translated or retryable properties already, they would not overwrite those values set in the constructor (true for both).

Maybe we should only update the retryable field if the original error doesn't have one (if someone passes an error with this already set should we honor it?). Since message is passed in explicitly, name is always MessagingError, and stack should indicate the line where this error is created, I don't think those should be overwritten.

Edit: I see that we preserve the stack when we convert an AmqpError to a MessagingError ...so depending on our intent maybe it's useful to preserve the stack at least.

Also, should be noted that this copies properties if they don't exist on MessagingError, not if they do.

Since we're wrapping errors and re-throwing, copying the stack property over might be more correct. Just make sure the stack that comes out is maximally useful. Aside from that, I think I agree with your intiution above - keep retryable if the original error had one, and message and name don't need to be copied.

I made a late edit that it might be better to copy the stack over as well, so I'll go ahead and make that change.

bterlson

Overall seems good to me. Left some comments.

bterlson · 2020-01-02T21:40:13Z

sdk/core/core-amqp/src/errors.ts

+  /**
+   * The amqp error condition.
+   */
+  amqpCondition?: string;


Thoughts about not exposing amqpCondition for now? I'm not entirely convinced it's helpful beyond the code, but do try to convince me 😀

@chradek had a good idea: log the amqp error instead.

Hmm, now I'm less sure about just logging the amqp error since we will want to log the translated error as well (that tells us if it is retryable), that would make the logs noisy since in many cases both errors would be logged to verbose mode.

I can still do that, but what about just logging condition alongside the translated error? We could also leave it out for now, there only appear to be 2 cases where 2 amqp errors map to the same code: UnauthorizedError and PreconditionFailedError and I'm not sure how useful it would be to know the specific condition in those cases.

bterlson · 2020-01-02T21:53:24Z

sdk/core/core-amqp/src/errors.ts

@@ -464,9 +468,23 @@ export class MessagingError extends Error {
  info?: any;
  /**
   * @param {string} message The error message that provides more information about the error.
+   * @param originalError An error whose properties will be copied to the MessagingError if the
+   * propert doesn't already exist.


Since we're wrapping errors and re-throwing, copying the stack property over might be more correct. Just make sure the stack that comes out is maximally useful. Aside from that, I think I agree with your intiution above - keep retryable if the original error had one, and message and name don't need to be copied.

bterlson · 2020-01-02T22:06:58Z

sdk/core/core-amqp/src/errors.ts

+      return;
+    }
+
+    // can consider using a mixin instead but not sure this gives us much value over casting to any


Yeah... not so much a big fan of the sloppy types here. It seems good to know which properties we will copy and which we won't. Not sure what you mean by mixin though?

I struggled with what to do here since I wanted to make sure we preserved anything the original error may have had even if we didn't know about it.

That said, we're now only converting AmqpErrors and SystemError, so I could add the properties from SystemError to MessagingError as optional properties:

address If present, the address to which a network connection failed

code The string error code

dest If present, the file path destination when reporting a file system error

errno | The system-provided error number

info If present, extra details about the error condition
message A system-provided human-readable description of the error

path If present, the file path when reporting a file system error

port If present, the network connection port that is not available

syscall The name of the system call that triggered the error

Edit: I can probably omit the filesystem-related properties as well to make the list shorter.

I got rid of the use of any here.

bterlson · 2020-01-02T22:09:29Z

sdk/core/core-amqp/src/errors.ts

@@ -519,8 +537,10 @@ export function isSystemError(err: any): boolean {
  if (
    err.code &&


Can we factor this massive conditional out of the if's test expression?

I can remove the if statement and just return the result instead? 😄

I'll make this more readable.

sdk/core/core-amqp/src/errors.ts

bterlson · 2020-01-02T22:19:44Z

sdk/core/core-amqp/src/errors.ts

    if (condition) {
-      const amqpErrorCondition = SystemErrorConditionMapper[condition as keyof typeof SystemErrorConditionMapper];
-      error.name = ConditionErrorNameMapper[amqpErrorCondition as keyof typeof ConditionErrorNameMapper];
+      const amqpErrorCondition =


Idle speculation: if this cast is valid, then perhaps code could be typed as keyof typeof SystemErrorConditionMapper to begin with?

bterlson · 2020-01-02T22:23:28Z

sdk/core/core-amqp/test/errors.spec.ts

-      translatedError.name.should.equal(ehError.name);
-      translatedError.retryable.should.equal(ehError.retryable);
-      translatedError.message.should.equal(ehError.message);
+    it("Does not touch errors that are neither AmqpError or SystemError", function() {


either neither and nor or either and or (so not neither and or or either and nor).

I'll make the change, though this took me on a fun journey :).

sdk/core/core-amqp/src/errors.ts

sdk/core/core-amqp/test/errors.spec.ts

sdk/core/core-amqp/src/errors.ts

sdk/core/core-amqp/CHANGELOG.md

ramya-rao-a · 2020-01-06T04:19:47Z

sdk/core/core-amqp/test/errors.spec.ts

+      const testError = new RangeError("Out of range!!");
+      const translatedError = Errors.translate(testError);
+      translatedError.name.should.equal(testError.name);
+      translatedError.message.should.equal(testError.message);


Since the above 3 error cases should go untouched, can we do a deep check to ensure that the input and the output for translate() is exactly the same?

Updated to use deep equality.

ramya-rao-a · 2020-01-06T06:01:59Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-01-06T06:02:10Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a

The integration tests for Event Hubs using core-amqp from this PR is hung.

Few follow ups needed to have the tests passing for Event Hubs

Test code that checks for error.name to be updated to use error.code instead
The err argument for processError is of type Error. Consider using Error | MessagingError instead.
Ensure the integration tests for Event Hubs all pass.

ramya-rao-a · 2020-01-06T07:38:34Z

Pushed a commit to make the change from name to code.

ramya-rao-a · 2020-01-06T07:39:26Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-01-06T07:39:35Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a · 2020-01-06T16:19:59Z

/azp run js - eventhubs-client - tests

azure-pipelines · 2020-01-06T16:20:12Z

Azure Pipelines successfully started running 1 pipeline(s).

[core-amqp] MessagingError changes

b1c9f1c

ramya-rao-a reviewed Jan 2, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 2, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Show resolved Hide resolved

ramya-rao-a reviewed Jan 2, 2020

View reviewed changes

chradek added 2 commits January 2, 2020 13:54

fix code in websocket error

2c7f284

fix AmqpError test with unknown condition

492cd3d

bterlson reviewed Jan 2, 2020

View reviewed changes

chradek added 4 commits January 2, 2020 15:17

refactor isSystemError for readability

f32a0fb

changed neither and or to neither and nor though either are fine

5af083b

cleanup property copying and remove amqpCondition

11baf23

update event hubs to cast MessagingError type

8fb7291

chradek marked this pull request as ready for review January 3, 2020 00:09

chradek requested a review from richardpark-msft as a code owner January 3, 2020 00:09

fix doc

6244996

ramya-rao-a reviewed Jan 3, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 3, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 3, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

add rhea-promise error translation

ea9082f

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/test/errors.spec.ts Outdated Show resolved Hide resolved

refactor SystemError tests

c037379

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

address feedback

7566a3e

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

missed one extraneous cast

14fb58c

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Show resolved Hide resolved

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/src/errors.ts Outdated Show resolved Hide resolved

chradek added 2 commits January 5, 2020 19:37

updated docs and isSystemError check

3a80222

remove extraneous err.stack copy from isSystemError block

b701519

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

sdk/core/core-amqp/CHANGELOG.md Outdated Show resolved Hide resolved

update changelog

e5f327f

ramya-rao-a reviewed Jan 6, 2020

View reviewed changes

ramya-rao-a approved these changes Jan 6, 2020

View reviewed changes

chradek and others added 3 commits January 5, 2020 20:32

update error tests to use deep equal

fb5dcea

Merge remote-tracking branch 'origin' into pr/chradek/6673

04cfa22

Fix unused import remaining for bad merge conflict resolution

1f95274

ramya-rao-a suggested changes Jan 6, 2020

View reviewed changes

Update event hubs to error code instead of name

270fcf4

ENOTFOUND error in node

1d7fa5a

ramya-rao-a approved these changes Jan 6, 2020

View reviewed changes

chradek merged commit 099e515 into Azure:master Jan 6, 2020

chradek deleted the core-amqp-messaging-error-changes branch January 6, 2020 17:03

ramya-rao-a mentioned this pull request Jan 14, 2020

[Service Bus] Update to use latest core-amqp #6861

Merged

		@@ -519,8 +537,10 @@ export function isSystemError(err: any): boolean {
		if (
		err.code &&

[core-amqp] MessagingError changes #6673

[core-amqp] MessagingError changes #6673

Conversation

chradek commented Dec 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a Jan 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bterlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chradek Jan 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramya-rao-a commented Jan 6, 2020

azure-pipelines bot commented Jan 6, 2020

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya-rao-a commented Jan 6, 2020 • edited Loading

ramya-rao-a commented Jan 6, 2020

azure-pipelines bot commented Jan 6, 2020

ramya-rao-a commented Jan 6, 2020

azure-pipelines bot commented Jan 6, 2020

ramya-rao-a Jan 6, 2020 •

edited

Loading

ramya-rao-a Jan 6, 2020 •

edited

Loading

chradek Jan 2, 2020 •

edited

Loading

chradek Jan 2, 2020 •

edited

Loading

ramya-rao-a commented Jan 6, 2020 •

edited

Loading