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

Rewind the request body for retries #194

Merged
merged 2 commits into from
Sep 27, 2023

Conversation

srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Aug 20, 2023

Fixes #193

prepareRequest in HTTP Sender returns a http.Request

func (h *HTTPSender) prepareRequest(ctx context.Context) (*http.Request, error) {

We have the backoff retry strategy for failed requests; however, underlying transport would close the request body and should be rewound for any subsequent attempt. This creates a thin wrapper around the http.Request that helps to rewind the body before making a request. We hijack and close the underlying connection to simulate the connection error case.

@srikanthccv srikanthccv requested a review from a team August 20, 2023 11:18
@codecov
Copy link

codecov bot commented Aug 20, 2023

client/internal/httpsender_test.go Outdated Show resolved Hide resolved
client/internal/httpsender_test.go Outdated Show resolved Hide resolved
@tigrannajaryan
Copy link
Member

Since the code has changed, do you mind squashing into one commit and updating the PR description to reflect what the code does? I will then do another review pass.

@srikanthccv
Copy link
Member Author

Updated, PTAL.

wg.Done()
}()
go func() {
time.Sleep(200 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned this won't be completely reliable. My understanding is that this is intended to wait for the request to fail before starting a server, is there a way we can wait for the OnConnectFailed callback to fire before starting it?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this when trying to create a failed test based on this discussion #194 (comment). This is no longer needed because the connection refused error doesn't reproduce the original issue so I updated the test to create a connection reset issue. Hope this is clear.

@tigrannajaryan tigrannajaryan enabled auto-merge (squash) September 21, 2023 21:29
@phanidevavarapu
Copy link
Contributor

@tigrannajaryan @srikanthccv are we good to merge this? I also started facing the similar issue.

@tigrannajaryan tigrannajaryan merged commit 816bc48 into open-telemetry:main Sep 27, 2023
5 checks passed
codeboten referenced this pull request in open-telemetry/opentelemetry-collector-contrib Nov 28, 2023
#29475)

[![Mend Renovate logo
banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[github.com/open-telemetry/opamp-go](https://github.com/open-telemetry/opamp-go)
| require | minor | `v0.9.0` -> `v0.10.0` |
|
[github.com/open-telemetry/opamp-go](https://github.com/open-telemetry/opamp-go)
| require | minor | `v0.8.0` -> `v0.10.0` |

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>open-telemetry/opamp-go
(github.com/open-telemetry/opamp-go)</summary>

###
[`v0.10.0`](https://github.com/open-telemetry/opamp-go/releases/tag/v0.10.0)

[Compare
Source](https://github.com/open-telemetry/opamp-go/compare/v0.9.0...v0.10.0)

##### What's Changed

- Move certs & helper code from /internal/examples to /internal by
[@&#8203;srikanthccv](https://github.com/srikanthccv) in
[https://github.com/open-telemetry/opamp-go/pull/201](https://github.com/open-telemetry/opamp-go/pull/201)
- Rewind the request body for retries by
[@&#8203;srikanthccv](https://github.com/srikanthccv) in
[https://github.com/open-telemetry/opamp-go/pull/194](https://github.com/open-telemetry/opamp-go/pull/194)
- Update to OpAMP spec 0.8.0 by
[@&#8203;tigrannajaryan](https://github.com/tigrannajaryan) in
[https://github.com/open-telemetry/opamp-go/pull/207](https://github.com/open-telemetry/opamp-go/pull/207)

**Full Changelog**:
open-telemetry/opamp-go@v0.9.0...v0.10.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these
updates again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

---------

Signed-off-by: Alex Boten <aboten@lightstep.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <107717825+opentelemetrybot@users.noreply.github.com>
Co-authored-by: Alex Boten <aboten@lightstep.com>
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.

httpsender will always fail to do retry request
4 participants