Use ddcommon connector in telemetry #6

pawelchcki · 2022-05-10T09:31:54Z

What does this PR do?

Extract common parts of hyper connector from ddprof-exporter and into ddcommon
Update ddtelemetry to use Tokio reactor + use the newly-shared connector

Motivation

ddtelemetry is using reqwest which pulls in a lot of dependencies inflating the binary size, and does not handle UDS.
Unifying http connectors between shared components should improve this situation.

ddtelemetry/src/worker.rs

ivoanjo

I'm somewhat curious about the move of the connector to ddcommon.

To be clear, I don't mind it, but design-wise it looks like previously ddcommon was not intended to have such heavy dependencies, and we're changing that, so I wanted to make sure everyone is fine with it. (I am)

If that's the design direction, would it make sense to also move tag.rs there? It doesn't seem very profiling-specific.

I would also suggest filling up a bit the PR description -- it took me a bit of time to understand that this PR includes two things:

Extract common parts of connector from ddprof-exporter and into ddcommon
Update ddtelemetry to use the newly-shared connector

ivoanjo · 2022-05-17T09:48:42Z

ddprof-exporter/src/errors.rs

            Self::OperationTimedOut => "operation timed out",
-            Self::UnixSocketUnsupported => "unix sockets unsupported on windows",
-            Self::CannotEstablishTlsConnection => {
-                "cannot establish requested secure TLS connection"
-            }
-            Self::NoValidCertifacteRootsFound => {
-                "native tls couldn't find any valid certifacte roots"
-            }
            Self::UserRequestedCancellation => "operation cancelled by user",


Would it make sense to just have this entire file in ddcommon, rather than having two error different enums?

I only moved those errors that were relevant to the moved code. Timedout and InvalidUrl, seem to be only specific to the existing implementation of the exporter.

Indeed, but even if it's only used in the other scope, do you think it's worth the extra complexity of having two error enums instead of one?

Honestly I'm not sure - don't have much experience in making Rust errors manageable. I think it might be better if each crate has its own Error enum - or avoid the concept.

In short keep Error enum as close as possible to the code that emits them.

ddprof-exporter/src/lib.rs

pawelchcki · 2022-05-18T15:51:56Z

I'm somewhat curious about the move of the connector to ddcommon.

To be clear, I don't mind it, but design-wise it looks like previously ddcommon was not intended to have such heavy dependencies, and we're changing that, so I wanted to make sure everyone is fine with it. (I am)

/cc @morrisonlevi

ddcomon previously only held very simple implementation. Now the connector pulls in a lot of heavy deps because of hyper and tokio.
I think between telemetry and ddprof it doesn't matter. as both will want/need to use both - once that changes, connector and its dependencies could become hidden behind feature flag.

If that's the design direction, would it make sense to also move tag.rs there? It doesn't seem very profiling-specific.

I haven't looked at tags.rs before. I think once we'd need to use it in telemetry, then we can move it.

At this point IMO it can live inside ddprof-exporter. or at very least, since this PR is already a bit too long, it could be moved in a separate PR.

ivoanjo

Left a few final notes/suggestions/questions but it overall looks good from my side. I would suggest waiting for a final pass and approval from @morrisonlevi since I'm just a humble Rust noob :)

ddcommon/src/connector/errors.rs

ddprof-exporter/src/lib.rs

ivoanjo · 2022-05-19T08:22:56Z

ddtelemetry/src/config.rs

+impl FromEnv {
+    fn get_agent_base_url() -> String {
+        let agent_port = env::var(DD_AGENT_PORT)
+            .ok()
+            .and_then(|p| p.parse::<u16>().ok())
+            .unwrap_or(DEFAULT_AGENT_PORT);
+        let agent_host =
+            env::var(DD_AGENT_HOST).unwrap_or_else(|_| String::from(DEFAULT_AGENT_HOST));
+
+        format!("http://{}:{}", agent_host, agent_port)
    }

-    if let Ok(dd_site) = env::var("DD_SITE") {
-        if dd_site.is_empty() {
-            format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, DEFAULT_DD_SITE)
-        } else {
-            format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, dd_site)
+    fn get_intake_base_url() -> String {
+        if let Some(url) = env::var(DD_APM_TELEMETRY_DD_URL)
+            .ok()
+            .filter(|s| !s.is_empty())
+        {
+            return url;
        }
-    } else {
-        String::from(STAGING_INTAKE)
-    }
-}

-impl Config {
-    pub fn get() -> &'static Self {
-        lazy_static! {
-            static ref CFG: Config = Config::read_env_config();
+        if let Ok(dd_site) = env::var(DD_SITE) {
+            if dd_site.is_empty() {
+                format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, DEFAULT_DD_SITE)
+            } else {
+                format!("{}.{}", PROD_INTAKE_FORMAT_PREFIX, dd_site)
+            }
+        } else {
+            String::from(STAGING_INTAKE)


I'm curious, where does this fit in languages where agent, port, etc can be configured via code (as in Ruby)?

The current approach in the profiler ffi is that the libdatadog caller can specify these settings, and so currently for Ruby it's dd-trace-rb that parses all the configuration and provides it to libdatadog, but it seems like for ddtelemetry we're going in a different direction?

Our telemetry old FFI's had setter methods on opaque handles, and IIRC config object was handled similarly. The from_env config is mostly meant for standalone tools. But could be integrated into a language if its found useful.

…nector

pawelchcki · 2022-05-25T17:28:40Z

Thanks for the review @ivoanjo.

…nector

paullegranddc · 2022-05-30T16:59:22Z

ddtelemetry/src/worker.rs

        Ok(self.sender.try_send(TelemetryActions::Stop)?)
    }

+    pub fn cancel_requests_with_deadline(&self, deadline: Instant) {


We currently have three methods to invoke in case you want to stop

send_stop

cancel_requests_with_deadline

wait_for_shutdown

They do different things, but I feel like since there really is only one order you'd want to invoke things (send_stop -> deadline -> wait) we could factorize at least two them together. Not sure which ones though either send with deadline, or deadline with wait?

Not blocking though

Yeah, I think we'll need to look at this again, once some FFI (php or other) starts using it.

paullegranddc · 2022-05-30T17:03:42Z

I'm somewhat curious about the move of the connector to ddcommon.
To be clear, I don't mind it, but design-wise it looks like previously ddcommon was not intended to have such heavy dependencies, and we're changing that, so I wanted to make sure everyone is fine with it. (I am)

/cc @morrisonlevi

ddcomon previously only held very simple implementation. Now the connector pulls in a lot of heavy deps because of hyper and tokio. I think between telemetry and ddprof it doesn't matter. as both will want/need to use both - once that changes, connector and its dependencies could become hidden behind feature flag.

If that's the design direction, would it make sense to also move tag.rs there? It doesn't seem very profiling-specific.

I haven't looked at tags.rs before. I think once we'd need to use it in telemetry, then we can move it.

At this point IMO it can live inside ddprof-exporter. or at very least, since this PR is already a bit too long, it could be moved in a separate PR.

By the way, the tag.rs implementation was moved to ddcommon from the previous PR

pawelchcki · 2022-05-30T17:23:15Z

Thanks @paullegranddc

paullegranddc reviewed May 12, 2022

View reviewed changes

ddtelemetry/src/worker.rs Outdated Show resolved Hide resolved

ddtelemetry/src/worker.rs Outdated Show resolved Hide resolved

ddtelemetry/src/worker.rs Outdated Show resolved Hide resolved

pawelchcki force-pushed the pawel/use_ddcommon_connector branch 2 times, most recently from 03f22a3 to 78d8c26 Compare May 13, 2022 09:03

pawelchcki marked this pull request as ready for review May 13, 2022 09:04

pawelchcki requested review from a team as code owners May 13, 2022 09:04

pawelchcki requested a review from paullegranddc May 13, 2022 09:04

ivoanjo reviewed May 17, 2022

View reviewed changes

pawelchcki force-pushed the pawel/use_ddcommon_connector branch from 78d8c26 to 86da296 Compare May 18, 2022 13:54

pawelchcki requested a review from a team as a code owner May 18, 2022 13:54

pawelchcki force-pushed the pawel/use_ddcommon_connector branch 2 times, most recently from 78bd9ef to d4b04c9 Compare May 18, 2022 14:04

pawelchcki added 2 commits May 18, 2022 16:18

Use shared http client + make telemetry worker async

4a5b508

Update License-3rd party

3d1a67f

pawelchcki force-pushed the pawel/use_ddcommon_connector branch from d4b04c9 to 3d1a67f Compare May 18, 2022 14:19

Fix erros on windows

99f763a

pawelchcki commented May 18, 2022

View reviewed changes

ddprof-exporter/src/lib.rs Show resolved Hide resolved

pawelchcki requested review from ivoanjo and morrisonlevi May 18, 2022 15:52

ivoanjo approved these changes May 19, 2022

View reviewed changes

pawelchcki added 5 commits May 23, 2022 15:20

Merge branch 'main' into pawel/use_ddcommon_connector

13e7593

Merge remote-tracking branch 'libdd/main' into pawel/use_ddcommon_con…

1e4ed5b

…nector

small code cleanup

c2166ee

Improve missing certificate msg

e29e179

Fix clippy and fmt

3bde6d3

pawelchcki force-pushed the pawel/use_ddcommon_connector branch from 61264c2 to 3bde6d3 Compare May 25, 2022 17:19

Merge remote-tracking branch 'libdd/main' into pawel/use_ddcommon_con…

b37eb2b

…nector

paullegranddc approved these changes May 30, 2022

View reviewed changes

pawelchcki merged commit 932c09b into main May 30, 2022

pawelchcki deleted the pawel/use_ddcommon_connector branch May 30, 2022 17:23

Use ddcommon connector in telemetry #6

Use ddcommon connector in telemetry #6

Uh oh!

Conversation

pawelchcki commented May 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pawelchcki commented May 18, 2022

Uh oh!

ivoanjo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ivoanjo May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelchcki commented May 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paullegranddc commented May 30, 2022

Uh oh!

pawelchcki commented May 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pawelchcki commented May 10, 2022 •

edited

Loading

ivoanjo May 19, 2022 •

edited

Loading