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

Use addError to set parent exceptions instead of .exception #370

Closed
wants to merge 1 commit into from

Conversation

DavidTanner
Copy link
Contributor

Currently when an exception is added to a subsegment the Segment.exception doesn't get cleaned up. This causes errors when internal services try to stringify the exception.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@DavidTanner
Copy link
Contributor Author

This addresses things like apollographql/apollo-server#1433 on lambda where the Lambda XRay facade doesn't clean up the .exception value that the Subsegment assigns to it.

@willarmiros
Copy link
Contributor

@DavidTanner this appears to be a problem unique to Lambda and the facade segment, is that correct? While I agree that the segment.exception not being cleaned up is a bug, perhaps we should be cleaning it up in the facade segment's reset function which is called at the start of each invocation.

I'm just having a bit of a hard time reasoning about whether this change affects the behavior of addError, namely in this case where segment.exception is already defined and subsegment.addError will point its cause to the ID of the captured exception if the new exception is the same as the existing one.

@DavidTanner
Copy link
Contributor Author

@willarmiros in that case I think the right thing to do would be to check out the exception is in the segment. I can update the code to check for that.

@NathanielRN
Copy link
Contributor

Hey @DavidTanner ! Are you still interested in working on this PR? If you are please let us know when it's ready for a review 🙂

@DavidTanner DavidTanner closed this May 3, 2024
@DavidTanner DavidTanner deleted the lambdaCircular branch May 3, 2024 15:33
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.

None yet

3 participants