-
Notifications
You must be signed in to change notification settings - Fork 397
Transports: DRY HTTP Transport & Client classes #5099
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
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2025-12-02 22:11:17 UTC |
Typing analysisNote: Ignored files are excluded from the next sections.
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 20fab87 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
BenchmarksBenchmark execution time: 2025-12-03 20:38:20 Comparing candidate commit c87e32b in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 41 metrics, 2 unstable metrics. scenario:profiling - Allocations (profiling disabled)
scenario:profiling - Allocations (profiling enabled)
scenario:profiling - intern mixed existing and new
|
ac5f229 to
3108b35
Compare
8ba9902 to
f3dc3d2
Compare
ivoanjo
left a comment
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.
Thanks for picking this up! Left a few notes.
TBH I suspect in the long run it may be easier to start from scratch and rebuilding the support for the required feature-set VS slowly untangling the current mess back into a good design step by step.
But having said that, I'll take any improvement I can get.
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 (except send_request_impl in tests stuff)
ivoanjo
left a comment
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.
While ideally there's a few more cleanups I'd like to get in, me and @p-datadog spoke about it and I think this PR as-is is already an improvement, so leaving my 👍
This reverts commit c87e32b.
|
I will be addressing the comments in the future PRs, thank you for the feedback. |
What does this PR do?
Removes duplicated code from Transport classes and deletes most of the HTTP::Client classes.
This PR continues on the path started in #5095.
Most of the HTTP::Client classes were functionally identical, except for the Tracing::Traces one which had additional logic for API version downgrades. The difference between the Client classes was in
send_XXX_payloadmethods which were largely identical. They have been replaced by a singlesend_requestmethod with the XXX as the argument. Then, all of the HTTP::Client classes have been removed except for the Tracing::Traces one which derives from the core HTTP::Client.This PR moves also moves the downgrading methods to the core class because they will be used by DI in the next PR. Accordingly, some unit tests have been moved to core. Not all because I think there needs to be additional work to have mock APIs, since existing unit tests under Tracing::Traces utilize the APIs in Traces.
Finally, a base Transport class in core has been created with the API management code that was identical across all of the Transport classes in the various components. The per-component Transport classes remain since they still have unique methods but these now all derive from the core base class.
Motivation:
Making transport layer more legible to implement API downgrades for DI
Change log entry
None
Additional Notes:
How to test the change?
Existing tests