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

Detect wrapped url error timeout #473

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

ian-shim
Copy link
Contributor

@ian-shim ian-shim commented Apr 11, 2024

Why are these changes needed?

Current way of detecting url error with type assertion doesn't work if the err being matched is wrapping url error.
This PR updates it so that it matches a url error from the error tree and then calling Timeout from the matched instance.

playground: https://goplay.tools/snippet/reDY2KH7bUD

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@@ -137,7 +137,11 @@ func (e *EncodingStreamer) Start(ctx context.Context) error {
// ignore canceled errors because canceled encoding requests are normal
continue
}
e.logger.Error("error processing encoded blobs", "err", err)
if strings.Contains(err.Error(), "too many requests") {
e.logger.Warn("error processing encoded blobs", "err", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

downgraded this case as it happens quite often and it's safe to ignore

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove error from the Warning message. So what is the source of too many requests? Are we being rate limited when this occurs?

@ian-shim ian-shim marked this pull request as ready for review April 11, 2024 17:02
@@ -137,7 +137,11 @@ func (e *EncodingStreamer) Start(ctx context.Context) error {
// ignore canceled errors because canceled encoding requests are normal
continue
}
e.logger.Error("error processing encoded blobs", "err", err)
if strings.Contains(err.Error(), "too many requests") {
e.logger.Warn("error processing encoded blobs", "err", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove error from the Warning message. So what is the source of too many requests? Are we being rate limited when this occurs?

if strings.Contains(err.Error(), "too many requests") {
e.logger.Warn("error processing encoded blobs", "err", err)
} else {
e.logger.Error("error processing encoded blobs", "err", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it could be confusing to use the same message for a Warning (which is safe to ignore) vs Error which are not safe to ignore. Can we change the error message in a way that is more provocative call to action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the message for warning

@ian-shim ian-shim merged commit 7c4d927 into Layr-Labs:master Apr 11, 2024
5 checks passed
bxue-l2 pushed a commit to bxue-l2/eigenda that referenced this pull request Apr 11, 2024
bxue-l2 pushed a commit to bxue-l2/eigenda that referenced this pull request Apr 11, 2024
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