refactor: ddcommon-net+profiling with hyper 1 #748
+660
−216
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
morrisonlevi
force-pushed
the
levi/ddcommon-net
branch
from
dc5aeaa
to
8c09a72
Compare
danielsn
reviewed
Nov 20, 2024
| @@ -37,7 +37,7 @@ members = [ | |||
| "data-pipeline-ffi", | |||
| "ddsketch", | |||
| "tinybytes", | |||
| "dogstatsd-client", | |||
| "dogstatsd-client", "ddcommon-net", | |||
| ddcommon = { path = "../ddcommon" } | ||
| hex = { version = "0.4" } | ||
| http = { version = "1" } | ||
| hyper = { version = "1", default-features = false } |
| use std::borrow::Cow; | ||
| use std::ops::Deref; | ||
|
|
||
| // todo: why do we need to serialize and deserialize endpoints? |
There was a problem hiding this comment.
I believe crashtracker uses this: we spawn a collector, and then tell it where to send data by sending a Config struct as json over a pipe.
| /// - User agent | ||
| /// - Api key | ||
| /// - Container Id/Entity Id | ||
| pub fn into_request_builder( |
There was a problem hiding this comment.
⛏️ I'd expect into_ named functions to consume the object.
| Self { | ||
| url: uri, | ||
| api_key: None, | ||
| timeout_ms: 0, |
There was a problem hiding this comment.
Should this be DEFAULT_TIMEOUT?
| api_key: Some(api_key.into()), | ||
| ..Default::default() | ||
| }) | ||
| pub trait ProfilingEndpoint { |
|
|
||
| /// Creates a tokio runtime for the current thread. This is the expected | ||
| /// way to create a runtime used by this crate. | ||
| pub fn create_current_thread_runtime() -> io::Result<runtime::Runtime> { |
There was a problem hiding this comment.
Should we stash this in a thread_local?
| if uri.scheme_str() != Some("unix") || uri.scheme_str() != Some("windows") { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "URI scheme must be unix or windows", |
There was a problem hiding this comment.
Suggested change
| "URI scheme must be unix or windows", | |
| format!("URI scheme must be unix or windows: was {}", uri.scheme_str()), |
| if uri.scheme_str() != Some("https") { | ||
| return Err(Error::Io(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| "URI scheme must be https", |
There was a problem hiding this comment.
Suggested change
| "URI scheme must be https", | |
| format!("URI scheme must be https, was {}", uri.scheme_str()), |
| Some(scheme) => match scheme.as_str() { | ||
| "http" => send_http(request).await, | ||
| "https" => send_https(request).await, | ||
| #[cfg(unix)] |
There was a problem hiding this comment.
Do we eventually want to support this on Windows? https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
ddcommon-net, naming suggestions welcome. It has bits ofddcommonnetworking code which have been upgraded to hyper 1. Note it's not a direct 1:1 because hyper 1 changed a lot.datadog-profilingcrate has been updated to use it. Currently its tests pass for me locally. Thedatadog-profiling-ffiis currently broken.Motivation
The main motivation is to upgrade hyper from v0.14 to v1. There are a lot of changes when moving to v1. Some things do not have direct replacements, so upgrading all crates at once is difficult. I decided (with input from a few others) that it seemed sensible to try making a
ddcommon-netcrate that uses hyper 1, and then crates can migrate one-by-one fromddcommontoddcommon-net.Additional Notes
Please tell me if you have concerns about the approach, technical or otherwise. Here are some of mine:
datadog-profilingis the only crate using this for now, isddcommon-netreally going to be common? Maybe we should make anetmodule inside ofdatadog-profilinginstead and only extract a crate when there is at least one other crate migrated to hyper 1?ddcommon-net::compatandddcommon, mostly around Uri and Endpoint related things. Probably okay, not sure.How to test the change?
Don't. This is a WIP, I'll update this when it's ready for testing.