Add Live debugging & Remote Configuration #398

mellon85 · 2024-04-19T15:40:11Z

Adding live debugging support. It requires remote configuration to work at all, so to do proper integration tests of the functionality, that part is also in this big PR.

Dynamic configuration is a small part, just 50 lines of code, describing the structures it contains.

As to more specific changes to current code:

The sidecar is now per user (on windows it already uses the ConsoleSession), instead of per system.
- On Windows it was necessary as memory can only easily be shared within a ConsoleSession.
- On Linux it's now necessary as the notification mechanism for changes to the remote config are distributed via signals, which require either same euid or root-like capabilities.
Note AsBytes in ddcommon currently requires &'a AsBytes<'a>, while only &AsBytes<'a> (independent lifetime) is required.

General notes:

Code in remote-config/ is meant to be reusable and also accessible via FFI as needed. For most users the bare ConfigFetcher should be enough, and for some the SharedFetcher is needed. Fewest should actually need the MultiTargetFetcher. A FFi interface hasn't been defined yet as it wasn't needed:
- ConfigFetcher in its simplest usage is just one function with two callbacks (store, update) essentially. StoredFile should be a wrapper for a type native to the FFI user (a pointer probably).
- SharedFetcher is similar, but presents a run-loop. To be used in anyway multi-threaded applications, now three callbacks: store, update and on_fetched.
The sidecar will notify participating processes whenever remote config changes.
The sidecar uses shared memory to transmit the stuff received by the remote-config stuff.
live_debugger is:
- a lot of manual parsing logic (If some value is X, then these fields are applicable. If this field is present, then it means this, otherwise that etc.; things which aren't trivially described by just deserializing it into a struct).
- an evaluator for parsed expressions, i.e. walking the parsed AST and requesting values from the runtime.
- a log probe sender

TODOs:

pierotibou

Skimmed through the PR and have a few questions/comments:

First, are you handling agentless and if so why? It wasn't mandatory for the first implementation right? MVP FTW.

I don't understand well the sidecar code yet, but I assume there are some constants that should be attached to the session and taken from there. This could avoid passing this data around for new use cases (service name, env, version...)

Then, you could have made it easier on the reviewer by adding commits or separating into a few PRs. I assume it's because it's in draft, but you're missing a lot of tests. Doing the refacto must have been extra complicated without tests.

Very exciting work though, that opens a lot of doors. Thanks for doing it.

trace-protobuf/src/remoteconfig.rs

pierotibou · 2024-05-15T08:52:55Z

remote-config/Cargo.toml

@@ -0,0 +1,23 @@
+[package]


What's the size impact of having RC? Are all those dependencies already used in libdatadog?

Only sha2 isn't. The fat ones, like tokio, serde, serde_json and hyper are used already in other packages than the sidecar too.

remote-config/src/dynamic_configuration/data.rs

pierotibou · 2024-05-15T08:58:16Z

remote-config/src/fetch/fetcher.rs

+    }
+
+    /// Quite generic fetching implementation:
+    ///  - runs a request against the Remote Config Server,


Is this not going through the agent?

Server as in "a remote endpoint which serves things" - might be datadog backend, might be agent. The datadog backend isn't supported yet. But it would eventually be handled here though.

https://github.com/DataDog/libdatadog/pull/398/files#r1603207015

sidecar/src/log.rs

pierotibou · 2024-05-15T09:06:50Z

sidecar-ffi/src/lib.rs

+
+#[no_mangle]
+#[allow(clippy::missing_safety_doc)]
+pub unsafe extern "C" fn ddog_sidecar_set_remote_config_data(


I wonder if this should be orthogonal to RC.
Data used for RC could be used for dogstatsdclient, telemetry... So I assume they should be linked to a session more than to RC itself. This couples a bit things, but I think it simplifies the API

Yes, it should be orthogonal.

mellon85 · 2024-05-16T07:13:19Z

remote-config/src/fetch/fetcher.rs

+
+const PROD_INTAKE_SUBDOMAIN: &str = "config";
+
+/// Manages files.


Talking about files when referring to configurations can be misleading. It's a ConfigurationStore, replace the naming from file to config please

I took the name from message File from the proto definition. It's literally the contents of these File messages, which are meant processed here.

mellon85 · 2024-05-16T07:14:45Z

remote-config/src/fetch/fetcher.rs

+    type StoredFile;
+
+    /// A new, currently unknown file was received.
+    fn store(&self, version: u64, path: RemoteConfigPath, contents: Vec<u8>) -> anyhow::Result<Arc<Self::StoredFile>>;


I wouldn't make a distinction between the 2 calls, the store knows if it's already present or not, what's the different use case?

Actually, the ConfigFileStorage impl in shm_remote_config.rs does not maintain it. The implementation is very straightforward there. It's done that way to avoid tracking state and expiration manually in the FileStorage implementations - that task is up to the ConfigFetcherState.

mellon85 · 2024-05-16T07:19:03Z

remote-config/src/fetch/fetcher.rs

+pub struct ConfigFetcher<S: FileStorage> {
+    pub file_storage: S,
+    state: Arc<ConfigFetcherState<S::StoredFile>>,
+    timeout: AtomicU32,


remote-config/src/fetch/fetcher.rs

mellon85 · 2024-05-16T08:59:39Z

remote-config/src/fetch/shared.rs

+    }
+}
+
+pub trait RefcountedFile {


Can't this be achieved by using weak and strong references?

No, using Weak references would imply that, once the fetchers no longer hold a reference to it, the underlying memory can no longer be accessed. This may not be desired; refcounting allows careful control of only relinquishing it from fetchers, without affecting live references.

I would give strong references to the shared clients and weak ones to everything else, so that it's self managed and doesn't have set methods setting a counter

remote-config/src/fetch/fetcher.rs

pierotibou · 2024-05-22T16:33:13Z

remote-config/Cargo.toml

nitpick - think of updating the codeowners for the new crates (remote-config and live-debugger). I assume common components could take ownership of the RC one.

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

bwoebi · 2024-06-19T20:37:10Z

Closing this PR in favor of the other #488.

github-actions bot added sidecar common labels Apr 19, 2024

bwoebi force-pushed the bob/live-debugger branch 6 times, most recently from 67d1d75 to 6808d58 Compare May 14, 2024 23:50

bwoebi changed the title ~~Bob/live debugger~~ Add Live debugging & Remote Configuration May 14, 2024

pierotibou reviewed May 15, 2024

View reviewed changes

bwoebi mentioned this pull request May 15, 2024

Provide a DogStatsD API in the sidecar #399

Merged

2 tasks

mellon85 commented May 16, 2024

View reviewed changes

VianneyRuhlmann reviewed May 17, 2024

View reviewed changes

remote-config/src/fetch/fetcher.rs Outdated Show resolved Hide resolved

bwoebi force-pushed the bob/live-debugger branch from 6808d58 to 07e897a Compare May 17, 2024 17:27

pierotibou reviewed May 22, 2024

View reviewed changes

bwoebi force-pushed the bob/live-debugger branch 3 times, most recently from 9562b23 to 1470cc9 Compare May 30, 2024 12:35

bwoebi and others added 5 commits June 11, 2024 16:52

Remote configuration & live debugging

bf98ad9

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

More dynamic configs, fixes and address several review comments

8ac495e

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

Add tests

4f148e5

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

Windows support

06fb35e

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

Bump rustls version to 0.23

568373c

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

bwoebi force-pushed the bob/live-debugger branch from 1470cc9 to 568373c Compare June 11, 2024 14:55

github-actions bot added the mini-agent label Jun 11, 2024

Refactor and test parsing and evaluating

732ef0e

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

bwoebi closed this Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Live debugging & Remote Configuration #398

Add Live debugging & Remote Configuration #398

mellon85 commented Apr 19, 2024 •

edited by bwoebi

Loading

pierotibou left a comment

pierotibou May 15, 2024

bwoebi May 15, 2024

pierotibou May 15, 2024

bwoebi May 15, 2024

mellon85 May 16, 2024

pierotibou May 15, 2024

bwoebi May 15, 2024

mellon85 May 16, 2024

bwoebi May 16, 2024

mellon85 May 16, 2024

bwoebi May 16, 2024

mellon85 May 16, 2024

mellon85 May 16, 2024

bwoebi May 16, 2024

mellon85 May 16, 2024

pierotibou May 22, 2024

bwoebi commented Jun 19, 2024


		const PROD_INTAKE_SUBDOMAIN: &str = "config";

		/// Manages files.

Add Live debugging & Remote Configuration #398

Add Live debugging & Remote Configuration #398

Conversation

mellon85 commented Apr 19, 2024 • edited by bwoebi Loading

pierotibou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwoebi commented Jun 19, 2024

mellon85 commented Apr 19, 2024 •

edited by bwoebi

Loading