-
Notifications
You must be signed in to change notification settings - Fork 471
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
Use bytes::Bytes as the HTTP request body in HttpClient. #2485
Use bytes::Bytes as the HTTP request body in HttpClient. #2485
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2485 +/- ##
=======================================
- Coverage 79.6% 79.6% -0.1%
=======================================
Files 118 118
Lines 22464 22467 +3
=======================================
Hits 17897 17897
- Misses 4567 4570 +3 ☔ View full report in Codecov by Sentry. |
Hello @cijothomas |
This will allow pooling of buffers on caller side compared to usage of Vec<u8>.
2e19563
to
e0156e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I believe it is safe to have dependency on Bytes crate (as of now, it was only used for header injection), as it well maintained, and widely used (eg, by tokio, reqwest, and hyper).
And actually |
Is there anything I can do to facilitate merging this? |
Sorry for the delay! Missed this un-intentionally! Would you be sending a PR to do buffer pooling for otlp? |
I don't have prepared PR to implement pooling for oltp, I've planned to implement it for Datadog (in contrib repo), because I have app which uses Datadog exporter and I see in profiler it will benefit if pooling. Regarding oltp I could try implementing it, but not very soon. |
I think Datadog now supports OTLP natively, so you should be able to leverage OTLP Exporter. We were discussing the possibility of sunsetting the DD exporter in favor of OTLP (can't find that discussion anymore :( ) @scottgerring @julianocosta89 shared this doc : https://www.datadoghq.com/blog/monitor-rust-otel/ |
Hello hello, just for reference, this thread had the initial discussion: https://cloud-native.slack.com/archives/C03GDP0H023/p1737396437600899 |
This will allow pooling of buffers on caller side compared to usage of Vec.
Fixes
Lack of possibility to reuse buffers which are consumed by
HttpClient
after request has been sent.Design discussion issue
Should old function be kept with deprecated annotation and hence new function had to have a different name or should I just introduce breaking change by changing signature of a function?
Changes
I propose to change type of body of http request accepted by
HttpClient
trait fromVec<u8>
tobytes::Bytes
.This should unblock pooling of buffers which can be used both for serialization & being http request bodies.
Something like this (pseudo-code to get the idea):
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes