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

aws/request: Fix duplicate request attempt metric reporting #2327

Merged
merged 2 commits into from Dec 5, 2018

Conversation

Projects
None yet
2 participants
@jasdel
Copy link
Member

commented Dec 5, 2018

Fixes duplicate request attempt metrics being reported when the request fails or retries.

@jasdel jasdel added the needs-tests label Dec 5, 2018

@jasdel jasdel self-assigned this Dec 5, 2018

aws/request: Fix duplicate request attempt metric reporting
Fixes duplicate request attempt metrics being reported when the request
fails or retrys.

@jasdel jasdel force-pushed the jasdel:fixup/CSMDuplicateAttempts branch from b6c8b72 to 1709bf4 Dec 5, 2018

@jasdel jasdel added review-needed and removed needs-tests labels Dec 5, 2018

}

func (r *Request) sendRequest() (sendErr error) {
defer r.Handlers.CompleteAttempt.Run(r)

This comment has been minimized.

Copy link
@bretambrose

bretambrose Dec 5, 2018

In chime you mentioned

"But... in the case of error the information needed isn't available until Retry handlers finishes"

It looks like we're invoking the new Complete handler before the Retry handlers. Will all the required error information be present at that time with this new control flow?

This comment has been minimized.

Copy link
@jasdel

jasdel Dec 5, 2018

Author Member

I was wrong with the original understanding of the order these operations needed to occur in. The previous implementation also sent the apiCallAttempt metric prior to the RetryAfter's handler was executed, because the csm reporter's apiCallAttempt handler was added to RetryAfter handler list before the SDK's retry delay.

In the case of a succesful request the RetryAfter handler would of never executed.

@jasdel

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

Investigating a potential timing bug in the updated request send. The Go 1.9 test timedout in what looks like the S3 package.

@jasdel

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

The 1.9 failure looks to be a transient failure, unrelated to this change.

@jasdel jasdel removed the review-needed label Dec 5, 2018

@jasdel jasdel merged commit 2f0155a into aws:master Dec 5, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@jasdel jasdel deleted the jasdel:fixup/CSMDuplicateAttempts branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.