Paullgdc/switch ddprof http client #32

paullegranddc · 2022-02-28T15:47:49Z

What does this PR do?

Switch the exporter http client from reqwest to hyper
Copy the MultipartForm format code from reqwest, for use it in the exporter.
// TODO what does this imply in term of licensing, do we need to add the link to the original license Apache/MIT somewhere?
Add UDS support for the exporter, by implementing hyper's Service<Uri> trait to connect to UDS sockets.

Sadly, tokio (and the rust std library btw) don't support UDS on non unix systems (such as windows) so the codepaths supporting sockets are enabled conditionally.

Unix sockets paths have to be provided in the following format unix://<path to the socket format> (eg. unix://./path/to/socket.sock) to be recognised by the library.

Motivation

This PR is motivated by a few things

reqwest doesn't support UDS. There are multiple years old issues open on the repo asking if it is planned.
The reqwest crate is the biggest contributor in term of size to the final artefact. By using a lower level we can remove a lot of the bloat generated (691.3KiB on 8.9MiB). The new artifact is 5.7MiB which is a 35% size reduction

Additional Notes

hyper doesn't support TLS natively. I added support for it through the hyper-rustls crate, but by default the rustls feature for request uses webpki as root of trust, whereas with hyper-rustls the default certificates are picked from the OS store which is probably is also why we are saving space.

How to test the change?

The multipart form implementation from reqwest had unit tests that I copied, and are passing.
I have tested the changes with the http_transport integration tests from the ruby profiler.

* Take the form multipart implementation from reqwest and adapt it to use with hyper

r1viollet · 2022-03-01T12:14:22Z

ddprof-exporter/src/multipart.rs

@@ -0,0 +1,741 @@
+use std::borrow::Cow;


were you able to take this from reqwest ?
Asking to understand how much effort we should spend reviewing this 😅

Yeah, all this part is taken from reqwest, with additions and types adjustments because it used reqwest::Body, which I switched for hyper::Body.

I can probably separate my additions, from the files I copied to make it easier to review.

I think it is fine as long as we are aware of it. Leaving the comment for future reviewers. Thank you !

Was this looked at and deemed unsuitable? https://github.com/ferristseng/rust-multipart-rfc7578

Actually no, I somehow missed it when looking for multipart libraires for hyper (now it's the first result on google??).
The code looks ok and it works with hyper 0.14 which wasn't the case of other libraires I look into, so I can try it and check if it's suitable.

I checked it and it doesn't work. rust-multipart-rfc7578 writes the final boundary of the form after each part, which is clearly a bug.

So for instance if multiple parts are written in the body, this lib emit the following body

content-type: multipart/form-data; boundary=abc --abc content-type: text/plain content-disposition: form-data; name="version" 3 --abc-- --abc content-type: text/plain content-disposition: form-data; name="start" 2022-02-07T15:59:53Z --abc-- --abc content-type: text/plain content-disposition: form-data; name="end" 2023-11-11T16:00:00Z --abc--

When it should be

content-type: multipart/form-data; boundary=abc --abc content-type: text/plain content-disposition: form-data; name="version" 3 --abc content-type: text/plain content-disposition: form-data; name="start" 2022-02-07T15:59:53Z --abc content-type: text/plain content-disposition: form-data; name="end" 2023-11-11T16:00:00Z --abc--

I would personally prefer that we contribute upstream and use something else than maintain it directly ourselves. At the very least, can you file an issue for them?

I agree that we don't want to maintain this ourselves.
Reviewing the code to find the root of the issue, I found a bunch of other problems, but the fixes weren't too bad, so I opened a couple PR on the repo .

ferristseng/rust-multipart-rfc7578#30
ferristseng/rust-multipart-rfc7578#31

The maintainer seems responsive, and the first one got merged and released very quickly.
In the meantime, I added a dependency to my fork in the Cargo.toml to verify that the ruby integration tests were still passing.

morrisonlevi · 2022-03-09T17:12:01Z

// TODO what does this imply in term of licensing, do we need to add the link to the original license Apache/MIT somewhere?

Yes, but I'm not sure where. The 3rd-party file makes sense but that's entirely auto-generated -- maybe we have some template that appends to instead of creates from scratch?

Edit: ah, 🤞🏻 the licensing issue will just go away because of the hyper-multipart fixes you made :)

ivoanjo · 2022-03-11T09:23:05Z

@morrisonlevi asked me to check if my change in #33 impacted this PR.

I've merged both changes locally and it still works fine 👍

I needed to do the following change:

diff --git a/ddprof-exporter/Cargo.toml b/ddprof-exporter/Cargo.toml
index a47a039..c3402bf 100644
--- a/ddprof-exporter/Cargo.toml
+++ b/ddprof-exporter/Cargo.toml
@@ -28,7 +28,7 @@ http-body = "0.4"
 pin-project-lite = "0.2.0"
 hyper-rustls = { version = "0.23", default-features = false, features = ["native-tokio", "http1"] }
 hex = "0.4"
-hyper-multipart-rfc7578 = { git = "https://github.com/paullegranddc/rust-multipart-rfc7578.git", rev = "0e0812e0241601a46986c062d1c4ba8574f437a5" }
+hyper-multipart-rfc7578 = { git = "https://github.com/paullegranddc/rust-multipart-rfc7578.git", rev = "8dcedc266e50876c04c91d24390fe9ac44f10b96" }

 [dev-dependencies]
 maplit = "1.0"

to get it to compile, otherwise I got

    Updating git repository `https://github.com/paullegranddc/rust-multipart-rfc7578.git`
error: failed to get `hyper-multipart-rfc7578` as a dependency of package `ddprof-exporter v0.4.0-rc.1 (/Users/ivo.anjo/datadog/libddprof/ddprof-exporter)`

Caused by:
  failed to load source for dependency `hyper-multipart-rfc7578`

Caused by:
  Unable to update https://github.com/paullegranddc/rust-multipart-rfc7578.git?rev=0e0812e0241601a46986c062d1c4ba8574f437a5#0e0812e0

Caused by:
  object not found - no match for id (0e0812e0241601a46986c062d1c4ba8574f437a5); class=Odb (9); code=NotFound (-3)

I'm guessing the previous commit hash became outdated after changes to ferristseng/rust-multipart-rfc7578#31 .

ivoanjo · 2022-03-11T09:40:10Z

Following libddprof's weekly sync meeting discussion, I decided to test the current state of the https/tls support. I used the exporter.cpp example app from examples/ffi since it uses agentless mode by default, aka it tries to communicate with the profiling backend directly over https.

I was unfortunately, not able to get it to work. Here's what I got, both on macOS and Linux:

root@docker-desktop:/working/examples/ffi/exporter-linux-amd64# ./exporter ivoanjo-testing-linux
Failed to send profile: : (error trying to connect: received fatal alert: HandshakeFailure)

ivo.anjo@macieira:~/datadog/libddprof/examples/ffi$ ./exporter ivoanjo-testing-macos
Failed to send profile: : (error trying to connect: received fatal alert: HandshakeFailure

@paullegranddc is this known/expected? Is there any missing steps to make https work fine?

paullegranddc · 2022-03-11T12:38:00Z

I'm guessing the previous commit hash became outdated after changes to ferristseng/rust-multipart-rfc7578#31 .

Thanks!
I forgot to update the ref to the latest fork commit. Should be fixed.

paullegranddc · 2022-03-11T13:12:42Z

@ivoanjo Apparently by default rusttls will only use TLS 1.3 which is not supported by the event intake. I added the tls1.2 feature to the hyper-rustls crate, and now the exporter doesn't fail anymore on handshake.

I also had 400 errors because the pprof files part didn't include the filenames should now be fixed

ivoanjo · 2022-03-11T13:42:27Z

Thanks for looking into it!

I just rebuilt the branch again with your changes and can confirm it works great on both macOS and Linux :)

morrisonlevi

Let's do this!

ivoanjo · 2022-03-15T17:26:09Z

Awesome! My new favorite merged PR ;)

paullegranddc added 5 commits February 28, 2022 15:23

* Switch the http client from reqwest to hyper

12c5d61

* Take the form multipart implementation from reqwest and adapt it to use with hyper

Create a Connector that can handle uniux socket streams

b0ad5ff

Add rustls hyper support

752f47a

Add rustls hyper support

c93e952

Rename CLient to HttpClient to work around cbindgen

c76a7c3

r1viollet reviewed Mar 1, 2022

View reviewed changes

paullegranddc added 5 commits March 2, 2022 13:45

handle unix socket path correctly

5df3c97

Better error message when the request times out

183bc5d

Separate original code from adapter in multipart handling

a238062

Add tests for uds handling

8d517aa

skip uds feature on non unix systems because tokio doesn't support it

7667e3d

paullegranddc marked this pull request as ready for review March 2, 2022 15:30

Switch the multipart implementation to hyper-multipart-rfc7578

56e4f46

Merge branch 'main' into paullgdc/switch_ddprof_http_client

80fc982

paullegranddc added 2 commits March 11, 2022 13:32

Enable tls1.2 on rustls

12d795f

update rust-multipart-rfc7578 fork ref

8a18e2d

Add filenames to form part

ae479f6

morrisonlevi approved these changes Mar 15, 2022

View reviewed changes

morrisonlevi merged commit 6dd1f8b into main Mar 15, 2022

morrisonlevi deleted the paullgdc/switch_ddprof_http_client branch March 15, 2022 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Paullgdc/switch ddprof http client #32

Paullgdc/switch ddprof http client #32

paullegranddc commented Feb 28, 2022 •

edited

Loading

r1viollet Mar 1, 2022

paullegranddc Mar 1, 2022 •

edited

Loading

r1viollet Mar 1, 2022

morrisonlevi Mar 3, 2022

paullegranddc Mar 3, 2022

paullegranddc Mar 7, 2022

morrisonlevi Mar 8, 2022 •

edited

Loading

paullegranddc Mar 9, 2022

morrisonlevi commented Mar 9, 2022 •

edited

Loading

ivoanjo commented Mar 11, 2022

ivoanjo commented Mar 11, 2022

paullegranddc commented Mar 11, 2022

paullegranddc commented Mar 11, 2022 •

edited

Loading

ivoanjo commented Mar 11, 2022

morrisonlevi left a comment

ivoanjo commented Mar 15, 2022

Paullgdc/switch ddprof http client #32

Paullgdc/switch ddprof http client #32

Conversation

paullegranddc commented Feb 28, 2022 • edited Loading

What does this PR do?

Motivation

Additional Notes

How to test the change?

r1viollet Mar 1, 2022

Choose a reason for hiding this comment

paullegranddc Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

r1viollet Mar 1, 2022

Choose a reason for hiding this comment

morrisonlevi Mar 3, 2022

Choose a reason for hiding this comment

paullegranddc Mar 3, 2022

Choose a reason for hiding this comment

paullegranddc Mar 7, 2022

Choose a reason for hiding this comment

morrisonlevi Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

paullegranddc Mar 9, 2022

Choose a reason for hiding this comment

morrisonlevi commented Mar 9, 2022 • edited Loading

ivoanjo commented Mar 11, 2022

ivoanjo commented Mar 11, 2022

paullegranddc commented Mar 11, 2022

paullegranddc commented Mar 11, 2022 • edited Loading

ivoanjo commented Mar 11, 2022

morrisonlevi left a comment

Choose a reason for hiding this comment

ivoanjo commented Mar 15, 2022

paullegranddc commented Feb 28, 2022 •

edited

Loading

paullegranddc Mar 1, 2022 •

edited

Loading

morrisonlevi Mar 8, 2022 •

edited

Loading

morrisonlevi commented Mar 9, 2022 •

edited

Loading

paullegranddc commented Mar 11, 2022 •

edited

Loading