ref: Remove actix-web client, make reqwest the new http client #938

untitaker · 2021-02-24T10:53:21Z

Remove any use of actix-web's HTTP client. We still need to clean up error types and remove unnecessary abstractions, but this should alleviate most of the pain that comes from having to support two http client implementations. We should do the real refactor in a follow-up so as to not introduce accidental behavior changes new error grouping.

RaduW

looks good to me

jan-auer

Thanks, LGTM. A few more local simplifications suggested below.

jan-auer · 2021-02-24T13:20:21Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## Unreleased

+**Features**:
+
+- Relay now picks up HTTP proxies from environment variables. This is made possible by switching to a different HTTP client library. The undocumented `http._client` option has been removed. ([#938](https://github.com/getsentry/relay/pull/938))


Let's move the bit about the undocumented option to **Internal**.

jan-auer · 2021-02-24T13:24:03Z

relay-server/src/actors/upstream.rs

-            None => {
-                let mut builder = ClientRequest::build();
-                builder.method(request.method.clone()).uri(uri);
+        let mut builder = {


This looks like a redundant scope -- can you get rid of it?

jan-auer · 2021-02-24T13:24:56Z

relay-server/src/actors/upstream.rs

-
-                Box::new(future) as Box<dyn Future<Item = _, Error = _>>
-            }
+        let future = {


This is a redundant scope, again.

jan-auer · 2021-02-24T13:25:24Z

relay-server/src/actors/upstream.rs

+                .flatten()
+                .map(Response);
+
+            Box::new(future) as Box<dyn Future<Item = _, Error = _>>


Can get rid of the boxing.

jan-auer · 2021-02-24T13:26:44Z

relay-server/src/endpoints/forward.rs

-                // keep it enabled.
-                //
-                // Essentially this means that content negotiation is done twice, and the response
-                // body is first decompressed by reqwest, then re-compressed by actix-web.


Can we keep the two lines of this comment somewhere? This is good insight, yet it is not apparent from the code alone.

jan-auer · 2021-02-24T13:30:03Z

relay-server/src/http.rs

            }
-        }
+            .boxed_local()


Is this boxed_local actually required if you box the compat future? Same below.

you need to produce an Unpin type to call compat(), and the way to do so is to box the future. I don't really understand how that makes sense tbh but that is what the type constraints are.

jan-auer · 2021-02-24T13:33:27Z

relay-server/src/http.rs

+                )
+                .boxed_local()
+                .compat(),
+        ) as Box<dyn Future<Item = _, Error = _>>


The cast here is redundant.

As per getsentry/relay#938 this option no longer exists. Existing values will be ignored, however.

* master: (23 commits) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog meta(readme): Document development config for processing mode (#921) ...

* master: (42 commits) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog ...

* master: (120 commits) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) ...

* master: test(general): Introduce macros for traversing Annotated (#826) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930)

ref: Remove actix-web client

3267a8f

untitaker requested a review from a team February 24, 2021 10:53

add changelog

5d371ab

RaduW approved these changes Feb 24, 2021

View reviewed changes

jan-auer approved these changes Feb 24, 2021

View reviewed changes

apply review feedback

ac02423

untitaker merged commit 5aa8930 into master Feb 24, 2021

untitaker deleted the ref/remove-awc branch February 24, 2021 15:00

untitaker added a commit to getsentry/self-hosted that referenced this pull request Feb 24, 2021

fix(relay): Remove http._client usage

1863592

As per getsentry/relay#938 this option no longer exists. Existing values will be ignored, however.

untitaker mentioned this pull request Feb 24, 2021

fix(relay): Remove http._client usage getsentry/self-hosted#870

Merged

untitaker added a commit to getsentry/self-hosted that referenced this pull request Feb 24, 2021

fix(relay): Remove http._client usage (#870)

7365a03

As per getsentry/relay#938 this option no longer exists. Existing values will be ignored, however.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: Remove actix-web client, make reqwest the new http client #938

ref: Remove actix-web client, make reqwest the new http client #938

untitaker commented Feb 24, 2021

RaduW left a comment

jan-auer left a comment

jan-auer Feb 24, 2021

jan-auer Feb 24, 2021

jan-auer Feb 24, 2021

jan-auer Feb 24, 2021

jan-auer Feb 24, 2021

jan-auer Feb 24, 2021

untitaker Feb 24, 2021

jan-auer Feb 24, 2021

ref: Remove actix-web client, make reqwest the new http client #938

ref: Remove actix-web client, make reqwest the new http client #938

Conversation

untitaker commented Feb 24, 2021

RaduW left a comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment