Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for specifying fault flag when adding error #593

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

icholy
Copy link

@icholy icholy commented Jun 10, 2023

Issue #61

Description of changes:

This change enables setting an error without setting the fault flag.

Example:

segment.addError(err, false, false);

@icholy icholy requested a review from a team as a code owner June 10, 2023 19:12
*/

Segment.prototype.addError = function addError(err, remote) {
Segment.prototype.addError = function addError(err, remote, fault) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

@icholy icholy Nov 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carolabadeer the change is backwards compatible. The fault || fault === undefined condition is semantically equivalent to defaulting fault to true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants