Implement sending via sidecar #192

bwoebi · 2023-07-10T11:59:30Z

What does this PR do?

Endpoint is exposed via FFI
Sidecar gains an API to handle agent sampling rates independently of the backend (i.e. working with sidecar and without sidecar). This is handled via mapped (shared) memory pages.
- The goal is to have an as up-to-date version as possible, but be lock-free. This is achieved by just ignoring the shared data while writes to the shared memory are happening, falling back to a prior process-local copy.
Sidecar config is now targets an Endpoint, which can be dynamically manipulated to fit the target url, with a possible API key. (telemetry mock file targets file:// Endpoint)
Sidecar uses trace-* crates to send data to an agent or intake endpoint.
Some common paths between sidecar and trace-mini-agent have been factored out into trace-utils. @DataDog/libdatadog-serverless, please review that part.
File passing in ipc got a macro handling the boilerplate code for FileTransferHandles.
- Primary motivation is that I've been utterly confused how this works (I was expecting some magic recognizing a trait on some args or such alongside serde) until after a while I discovered that explicit impl TransferHandles for ExampleInterfaceRequest, for both, which is just trivial boilerplate. The macro makes it explicit in the service definition what is passed.
Global configuration is now directly passed via env upon sidecar creation. (guarding the self-telemetry for example)

The changes to the sidecar crate have been tested and validated in the dd-trace-php repository.

I'm aware the PR is quite big, but everything needed to come together to do proper end-to-end testing.

thedavl

Thanks for doing the re-factoring, I really like how you abstracted common logic. I left a couple questions regarding some stuff I wasn't clear on.

thedavl · 2023-07-12T14:36:12Z

trace-utils/src/trace_utils.rs

+            };
+
+            send_request(req, serialized_trace_payload, StatusCode::ACCEPTED).await
+        } else {


Could you explain this else block to me? Is it for sending traces to the trace agent from within the sidecar?

Yes :-)
The Agent just takes individual collections of trace chunk, not a whole payload.

thedavl · 2023-07-12T17:15:20Z

trace-utils/src/trace_utils.rs

-pub fn serialize_agent_payload(payload: pb::AgentPayload) -> anyhow::Result<Vec<u8>> {
+pub fn serialize_proto_payload<T>(payload: T) -> anyhow::Result<Vec<u8>>
+where
+    T: ::prost::Message,


just a general rust question, but what's the purpose of the prefix :: in ::prost::Message? Is this a stylistic choice, as it seems like it would work without it?

Can be probably removed, I believe, I copied it from the generated protobuf files (where it's done to avoid conflicts...).

thedavl · 2023-07-12T18:36:26Z

trace-utils/src/trace_utils.rs

+
+        let mut req = hyper::Request::builder()
+            .uri(target.url.clone())
+            .header(hyper::header::USER_AGENT, concat!("Tracer/", env!("CARGO_PKG_VERSION")))


what's this header used for?

Telemetry and profiler use the header to clearly distinguish which code is the sender, in case some things are observed on the receiving side so that it's obvious what code sends it.

thedavl

looks good on the serverless end, thanks for the refactoring!

The base branch was changed.

ddcommon-ffi/src/endpoint.rs

ddtelemetry/src/worker/http_client.rs

paullegranddc · 2023-07-31T12:51:50Z

ipc/macros/src/lib.rs

+        }
+    }
+
+    camel_ty.shrink_to_fit();


Is this necessary? I'd think that in term of perf the reallocation is worse than few extra bytes

This is macro-code though, so just compile time overhead.

ipc/src/transport/blocking.rs

trace-utils/src/trace_utils.rs

sidecar/src/interface.rs

sidecar/src/agent_remote_config.rs

Signed-off-by: Bob Weinand <[email protected]>

bwoebi · 2023-08-03T13:47:28Z

@paullegranddc Thanks for the review. I've addressed the comments in the newest commit. Also rebased on main to fix conflicts.

The only remaining question is: this CI failure https://github.com/DataDog/libdatadog/actions/runs/5751516424/job/15590430623?pr=192
Do you have any idea how I work around that?

paullegranddc · 2023-08-03T15:44:25Z

The only remaining question is: this CI failure https://github.com/DataDog/libdatadog/actions/runs/5751516424/job/15590430623?pr=192
Do you have any idea how I work around that

I think this is because you updated to nix 0.26 which calls to libc::memfd_create https://docs.rs/nix/latest/src/nix/sys/memfd.rs.html#56 and even though this function
centos 7 has a fairly old glibc (2.17), and memfd_create was only added in 2.28 so the linking process fails...

The maintainers are aware of the issue but it seems like they don't want to fix it nix-rust/nix#1972

Can you use nix 0.24 in the sidecar crate? (which is already used by the spawn_worker crate by the way)

paullegranddc · 2023-08-03T15:49:57Z

Another option they propose in the nix repo thread is to use full LTO for all builds, so that the code calling to libc::memfd_create is removed as dead code before the linking process since it does everything in a single compilation unit.

Or we could disable the memfd code on centos like we do for non linux os using features?

bwoebi · 2023-08-03T20:33:43Z

I'm not even using the memfd thing from nix. Trying to disable with features seemingly did not help, even though it's the fs feature if I see that correctly, which isn't used?

Signed-off-by: Bob Weinand <[email protected]>

ipc/src/platform/unix/mem_handle.rs

paullegranddc · 2023-08-04T15:27:41Z

sidecar/src/agent_remote_config.rs

+
+                // Ensure the next write hasn't started yet *and* the data is from the expected generation
+                if !new_data.meta.writing.load(Ordering::SeqCst)
+                    && new_generation == copied_data.meta.generation.load(Ordering::Acquire)


If you want to check that no new generation has happened while you were writing, you should probably load the the atomic here instead of relying on the one read at the beginning of the function.

The code was using the wrong data as source, needs to be new_data - the goal is checking whether the initial generation before I start copying is identical to the one after copying.

Signed-off-by: Bob Weinand <[email protected]>

* Implemented tracing and agent sampling in sidecar * Address CR feedback Signed-off-by: Bob Weinand <[email protected]> * Polyfill memfd on old glibc targets Signed-off-by: Bob Weinand <[email protected]> * Small nit from CR applied Signed-off-by: Bob Weinand <[email protected]> --------- Signed-off-by: Bob Weinand <[email protected]>

bwoebi requested review from pawelchcki and paullegranddc July 10, 2023 11:59

bwoebi requested review from a team as code owners July 10, 2023 11:59

github-actions bot added ci-build telemetry mini-agent sidecar common labels Jul 10, 2023

bwoebi force-pushed the bob/fix-telemetry-leaks branch from 19dbffb to 77b60c1 Compare July 10, 2023 13:29

bwoebi force-pushed the bob/sidecar-traces branch from e5b7de2 to 85bb04e Compare July 11, 2023 09:25

thedavl reviewed Jul 12, 2023

View reviewed changes

thedavl previously approved these changes Jul 13, 2023

View reviewed changes

bwoebi force-pushed the bob/sidecar-traces branch 2 times, most recently from c17b1f1 to 2cbbe1e Compare July 18, 2023 17:21

bwoebi changed the base branch from bob/fix-telemetry-leaks to main July 21, 2023 15:23

paullegranddc reviewed Aug 1, 2023

View reviewed changes

paullegranddc reviewed Aug 2, 2023

View reviewed changes

sidecar/src/agent_remote_config.rs Outdated Show resolved Hide resolved

paullegranddc reviewed Aug 2, 2023

View reviewed changes

sidecar/src/agent_remote_config.rs Outdated Show resolved Hide resolved

paullegranddc reviewed Aug 2, 2023

View reviewed changes

sidecar/src/agent_remote_config.rs Outdated Show resolved Hide resolved

paullegranddc reviewed Aug 2, 2023

View reviewed changes

sidecar/src/agent_remote_config.rs Outdated Show resolved Hide resolved

Implemented tracing and agent sampling in sidecar

458a87c

bwoebi force-pushed the bob/sidecar-traces branch 2 times, most recently from 857284f to 33705f1 Compare August 3, 2023 13:40

Address CR feedback

a0a697d

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the bob/sidecar-traces branch from 33705f1 to a0a697d Compare August 3, 2023 13:47

bwoebi force-pushed the bob/sidecar-traces branch from 02b8f4e to 6363b02 Compare August 3, 2023 20:23

bwoebi force-pushed the bob/sidecar-traces branch 6 times, most recently from b697a6f to 604b76e Compare August 4, 2023 13:57

Polyfill memfd on old glibc targets

e9f2698

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the bob/sidecar-traces branch from 604b76e to e9f2698 Compare August 4, 2023 14:00

paullegranddc previously approved these changes Aug 4, 2023

View reviewed changes

bwoebi dismissed paullegranddc’s stale review via 6794d7c August 4, 2023 16:00

Small nit from CR applied

c46f5a3

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the bob/sidecar-traces branch from 6794d7c to c46f5a3 Compare August 4, 2023 16:02

paullegranddc approved these changes Aug 4, 2023

View reviewed changes

bwoebi merged commit 2a605f1 into main Aug 4, 2023

bantonsson deleted the bob/sidecar-traces branch March 7, 2024 07:14

Implement sending via sidecar #192

Implement sending via sidecar #192

Uh oh!

Conversation

bwoebi commented Jul 10, 2023

What does this PR do?

Uh oh!

thedavl 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thedavl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bwoebi commented Aug 3, 2023

Uh oh!

paullegranddc commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paullegranddc commented Aug 3, 2023

Uh oh!

bwoebi commented Aug 3, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

paullegranddc commented Aug 3, 2023 •

edited

Loading