Skip to content
This repository was archived by the owner on Nov 18, 2025. It is now read-only.

feat: Add errorsEncountered to the errors sent back#1740

Merged
danieljbruce merged 32 commits intomainfrom
add-errors-encountered
Apr 29, 2025
Merged

feat: Add errorsEncountered to the errors sent back#1740
danieljbruce merged 32 commits intomainfrom
add-errors-encountered

Conversation

@danieljbruce
Copy link
Contributor

@danieljbruce danieljbruce commented Apr 25, 2025

Description

As the client retries, this change collects errors that are encountered. Then when an error is encountered that is not retried and that error reaches the user, it will contain information about all the errors that were encountered during a series of retries which will help identify what happened during a call with a handwritten client library.

Impact

Bugs like https://github.com/googleapis/nodejs-datastore/issues/1176 are not surfacing all errors encountered which makes them extremely hard to troubleshoot. As a result, we are not able to do root cause analysis because original errors are hard to find. This fix will help us troubleshoot ambiguous errors in our clients and find root causes.

Testing

None at the moment.

Additional Information

This adds information to the error details that the user will receive.

Next Steps

This change is just for normal calls. We can consider making this change for other call types as well.

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Apr 25, 2025
@danieljbruce danieljbruce marked this pull request as ready for review April 25, 2025 16:01
@danieljbruce danieljbruce requested a review from a team April 25, 2025 16:01
@danieljbruce danieljbruce requested a review from a team as a code owner April 25, 2025 16:01
@sofisl
Copy link
Contributor

sofisl commented Apr 25, 2025

I think it's worth adding a test, see similar tests I added to this logging here

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 25, 2025
@danieljbruce danieljbruce requested a review from sofisl April 28, 2025 18:12
@danieljbruce
Copy link
Contributor Author

@sofisl Ok. I added some tests. It's interesting that the test in gax/test/unit/apiCallable.ts passes when checking for Previous errors while the test in gax/test/test-application/src/index.ts also passes even though its error message doesn't include the Previous errors string at all. My suspicion is that the test in gax/test/test-application/src/index.ts in the referenced PR actually explores a different code path.

await client.attemptSequence(attemptRequest, settings);
assert.fail('The operation should have failed')
} catch (e) {
const expectedMessage = 'Total timeout of API google.showcase.v1beta1.SequenceService exceeded 1 milliseconds retrying error Error: 14 UNAVAILABLE: 14 before any response was received.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should try the test with different errors so that you can test your new functionality? Assuming you want something more than just the current error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR it is the unit test checking for the Previous errors string that is more critical. My suspicion is that this test doesn't even test the code path where the source code changes are. I think it targets streaming calls, but it's nice to have anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran an experiment in the other PR and have verified in the test run at https://github.com/googleapis/gax-nodejs/actions/runs/14716048863/job/41299549173?pr=1743 that the previous errors are getting included in the error message.

@danieljbruce danieljbruce requested a review from sofisl April 28, 2025 20:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants