Add support for specifying fault flag when adding error#593
Add support for specifying fault flag when adding error#593icholy wants to merge 1 commit intoaws:masterfrom
Conversation
| */ | ||
|
|
||
| Segment.prototype.addError = function addError(err, remote) { | ||
| Segment.prototype.addError = function addError(err, remote, fault) { |
There was a problem hiding this comment.
Changing this function signature will be a breaking / backwards-incompatible change. Ideally, we would want to maintain the current expected behaviour so that this change does not cause any unexpected changes for others as well.
I would recommend setting a default value of true for the fault parameter to ensure that the fault flag is always added by default, and would only be excluded if a third argument with the value of false is included.
I am also curious, what is the use case for excluding the fault flag? Is there a specific reason why it would not be included?
There was a problem hiding this comment.
@carolabadeer Here I would like to answer, as I am having the same need as @icholy. In the segments error documentation, is mentioned that the flags error, throttle and fault, can represent different types of errors.
In my personal use case, we are flagging all expected errors as error and unexpected errors as fault.
At the moment of catching and handling the error logic, we want to add error data to the segment with addError method, but it always set fault flag to true. So we need to overwrite the flag accordingly, whether the error is expected or unexpected.
An extra param as @icholy suggest will reduce extra code, and avoid overwriting the segment properties directly.
There was a problem hiding this comment.
@carolabadeer the change is backwards compatible. The fault || fault === undefined condition is semantically equivalent to defaulting fault to true.
Issue #61
Description of changes:
This change enables setting an error without setting the
faultflag.Example: