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.

Sign up for GitHub

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
Open

Add support for specifying fault flag when adding error #593

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/core/lib/segments/segment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,16 +280,19 @@ Segment.prototype.removeSubsegment = function removeSubsegment(subsegment) {
* Adds error data into the segment.
* @param {Error|string} err - The error to capture.
* @param {boolean} [remote] - Flag for whether the exception caught was remote or not.
* @param {boolean} [fault] - Flag for whether the fault flag should be set.
*/

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?

Copy link

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 •
edited
Loading

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.

if (err == null || typeof err !== 'object' && typeof(err) !== 'string') {
logger.getLogger().error('Failed to add error:' + err + ' to subsegment "' + this.name +
'". Not an object or string literal.');
return;
}

this.addFaultFlag();
if (fault || fault === undefined) {
this.addFaultFlag();
}

if (this.exception) {
if (err === this.exception.ex) {
Expand Down Expand Up @@ -381,15 +384,16 @@ Segment.prototype.decrementCounter = function decrementCounter() {
* Closes the current segment. This automatically sets the end time.
* @param {Error|string} [err] - The error to capture.
* @param {boolean} [remote] - Flag for whether the exception caught was remote or not.
* @param {boolean} [fault] - Flag for whether the fault flag should be set.
*/

Segment.prototype.close = function(err, remote) {
Segment.prototype.close = function(err, remote, fault) {
if (!this.end_time) {
this.end_time = SegmentUtils.getCurrentTime();
}

if (err !== undefined) {
this.addError(err, remote);
this.addError(err, remote, fault);
}

delete this.in_progress;
Expand Down