feat(console): add support for Unix domain sockets #388

benesch · 2022-12-04T13:15:56Z

Add support for console connections that use Unix domain sockets rather than TCP.

benesch · 2022-12-04T13:16:40Z

console-subscriber/src/builder.rs

+                    )
+                    .next()
+                    .expect("tokio console could not resolve TOKIO_CONSOLE_BIND"),
+            );


@hawkw do you think this should support UDS addresses too? E.g., by detecting the presence of a leading /, or the absence of a :port?

Hmm, my preference is to have this env var only configure a TCP socket, because it seems unfortunate to have a potential typo in the environment variable silently switch from TCP to UDS when that might not actually be desired.

Instead, I think we might want to consider adding a separate environment variable to configure serving over Unix sockets. It would be fine to add that env var in this branch, or in a follow-up PR.

Instead, I think we might want to consider adding a separate environment variable to configure serving over Unix sockets. It would be fine to add that env var in this branch, or in a follow-up PR.

I'll leave that to a different PR, then!

console-subscriber/src/builder.rs

hawkw

Thanks for working on this! This branch looks like a great start!

I had a few suggestions, most of which I also elaborated on in inline comments:

Unix-only APIs need #[cfg(unix)] attributes so that the console still compiles on Windows.
We should determine whether we want to allow serving the API on both a Unix domain socket and a TCP socket simultaneously, which might be desirable in some cases.
We probably want a separate env-var-based configuration for serving on UDS, if we even want to allow configuring that with the environment variable API.

Let's update this comment (which generates the CLI help text for tokio-console):

console/tokio-console/src/config.rs

Lines 25 to 29 in c7ce40f

    
               /// The address of a console-enabled process to connect to. 
        
               /// 
        
               /// This may be an IP address and port, or a DNS name. 
        
               /// 
        
               /// [default: http://127.0.0.1:6669]

so that it also states that the URL can be a Unix Domain Socket if it starts with file://, on unix platforms.

console-subscriber/src/builder.rs

hawkw · 2022-12-06T17:17:09Z

console-subscriber/src/builder.rs

+                    )
+                    .next()
+                    .expect("tokio console could not resolve TOKIO_CONSOLE_BIND"),
+            );


Hmm, my preference is to have this env var only configure a TCP socket, because it seems unfortunate to have a potential typo in the environment variable silently switch from TCP to UDS when that might not actually be desired.

Instead, I think we might want to consider adding a separate environment variable to configure serving over Unix sockets. It would be fine to add that env var in this branch, or in a follow-up PR.

console-subscriber/src/builder.rs

hawkw · 2022-12-06T17:21:59Z

console-subscriber/src/lib.rs

+use tokio::{
+    net::UnixListener,
+    sync::{mpsc, oneshot},
+};
+use tokio_stream::wrappers::UnixListenerStream;


i believe the UnixListener imports won't compile on operating systems other than Unix (e.g., on Windows). we probably need to add a cfg attribute so that this code builds on Windows.

Suggested change

use tokio::{

net::UnixListener,

sync::{mpsc, oneshot},

};

use tokio_stream::wrappers::UnixListenerStream;

use tokio::sync::{mpsc, oneshot};

#[cfg(unix)]

use tokio::net::UnixListener;

#[cfg(unix)]

use tokio_stream::wrappers::UnixListenerStream;

hawkw · 2022-12-06T17:22:56Z

console-subscriber/src/builder.rs

+    /// A Unix socket address.
+    Unix(PathBuf),


This should probably have #[cfg(unix)] so that the API is only available when building for Unix.

Suggested change

/// A Unix socket address.

Unix(PathBuf),

/// A Unix socket address.

#[cfg(unix)]

Unix(PathBuf),

hawkw · 2022-12-06T17:23:24Z

console-subscriber/src/builder.rs

+impl From<PathBuf> for ServerAddr {
+    fn from(path: PathBuf) -> ServerAddr {
+        ServerAddr::Unix(path)
+    }
+}
+
+impl<'a> From<&'a Path> for ServerAddr {
+    fn from(path: &'a Path) -> ServerAddr {
+        ServerAddr::Unix(path.to_path_buf())
+    }
+}


similarly, these probably need to be cfg-flagged:

Suggested change

impl From<PathBuf> for ServerAddr {

fn from(path: PathBuf) -> ServerAddr {

ServerAddr::Unix(path)

}

}

impl<'a> From<&'a Path> for ServerAddr {

fn from(path: &'a Path) -> ServerAddr {

ServerAddr::Unix(path.to_path_buf())

}

}

#[cfg(unix)]

impl From<PathBuf> for ServerAddr {

fn from(path: PathBuf) -> ServerAddr {

ServerAddr::Unix(path)

}

}

#[cfg(unix)]

impl<'a> From<&'a Path> for ServerAddr {

fn from(path: &'a Path) -> ServerAddr {

ServerAddr::Unix(path.to_path_buf())

}

}

console-subscriber/src/lib.rs

tokio-console/src/conn.rs

hawkw · 2022-12-06T17:25:55Z

tokio-console/src/conn.rs

+                    None | Some("file") => {
+                        // Dummy endpoint is ignored by the connector.
+                        let endpoint = Endpoint::from_static("http://localhost");
+                        let path = self.target.path().to_owned();
+                        endpoint
+                            .connect_with_connector(tower::service_fn(move |_| {
+                                UnixStream::connect(path.clone())
+                            }))
+                            .await?
+                    }


this match arm will need to be #[cfg(unix)], because it uses the Unix-only UnixStream type:

Suggested change

None | Some("file") => {

// Dummy endpoint is ignored by the connector.

let endpoint = Endpoint::from_static("http://localhost");

let path = self.target.path().to_owned();

endpoint

.connect_with_connector(tower::service_fn(move |_| {

UnixStream::connect(path.clone())

}))

.await?

}

#[cfg(unix)]

None | Some("file") => {

// Dummy endpoint is ignored by the connector.

let endpoint = Endpoint::from_static("http://localhost");

let path = self.target.path().to_owned();

endpoint

.connect_with_connector(tower::service_fn(move |_| {

UnixStream::connect(path.clone())

}))

.await?

}

also, we may want to add a separate #[cfg(not(unix))] match arm that prints an error message when trying to connect to a Unix domain socket on non-unix platforms?

hawkw · 2022-12-06T17:29:41Z

tokio-console/src/conn.rs

@@ -78,7 +82,23 @@ impl Connection {
                tokio::time::sleep(backoff).await;
            }
            let try_connect = async {
-                let mut client = InstrumentClient::connect(self.target.clone()).await?;
+                let channel = match self.target.scheme_str() {
+                    None | Some("file") => {


i'm not sure how i feel about treating URLs without a scheme as UDS...is that the generally accepted convention?

I don't think there is a generally accepted convention. Some other software uses unix: as the scheme, as in unix://path/to/socket. For now I just removed the treatment of None as UDS; let me know if you'd prefer some other scheme instead.

benesch

Thanks for the comments! I've addressed everything, I think!

console-subscriber/src/builder.rs

benesch · 2022-12-11T15:42:54Z

console-subscriber/src/builder.rs

+                    )
+                    .next()
+                    .expect("tokio console could not resolve TOKIO_CONSOLE_BIND"),
+            );


Instead, I think we might want to consider adding a separate environment variable to configure serving over Unix sockets. It would be fine to add that env var in this branch, or in a follow-up PR.

I'll leave that to a different PR, then!

console-subscriber/src/builder.rs

benesch · 2022-12-11T15:49:59Z

console-subscriber/src/builder.rs

+    /// A Unix socket address.
+    Unix(PathBuf),


benesch · 2022-12-11T15:50:07Z

console-subscriber/src/builder.rs

+impl From<PathBuf> for ServerAddr {
+    fn from(path: PathBuf) -> ServerAddr {
+        ServerAddr::Unix(path)
+    }
+}
+
+impl<'a> From<&'a Path> for ServerAddr {
+    fn from(path: &'a Path) -> ServerAddr {
+        ServerAddr::Unix(path.to_path_buf())
+    }
+}


benesch · 2022-12-11T15:50:10Z

console-subscriber/src/lib.rs

+use tokio::{
+    net::UnixListener,
+    sync::{mpsc, oneshot},
+};
+use tokio_stream::wrappers::UnixListenerStream;


console-subscriber/src/lib.rs

benesch · 2022-12-11T15:50:24Z

tokio-console/src/conn.rs

+                    None | Some("file") => {
+                        // Dummy endpoint is ignored by the connector.
+                        let endpoint = Endpoint::from_static("http://localhost");
+                        let path = self.target.path().to_owned();
+                        endpoint
+                            .connect_with_connector(tower::service_fn(move |_| {
+                                UnixStream::connect(path.clone())
+                            }))
+                            .await?
+                    }


benesch · 2022-12-11T16:08:41Z

tokio-console/src/conn.rs

@@ -78,7 +82,23 @@ impl Connection {
                tokio::time::sleep(backoff).await;
            }
            let try_connect = async {
-                let mut client = InstrumentClient::connect(self.target.clone()).await?;
+                let channel = match self.target.scheme_str() {
+                    None | Some("file") => {


I don't think there is a generally accepted convention. Some other software uses unix: as the scheme, as in unix://path/to/socket. For now I just removed the treatment of None as UDS; let me know if you'd prefer some other scheme instead.

benesch · 2022-12-16T02:03:50Z

@hawkw anything you're waiting on me on?

hawkw · 2022-12-16T17:34:28Z

@hawkw anything you're waiting on me on?

Nope, sorry, I just hadn't gotten the chance to give this a review yet. I'll try to do one today!

benesch · 2022-12-16T17:35:32Z

No problem! Just wanted to make sure I hadn't missed something. Feel free to just push directly at the branch (or open a new PR, whatever) if easier.

hawkw

Overall, this looks good to me! I had some last suggestions for improving the documentation. Once those are addressed, though, I'll be very happy to merge this!

console-subscriber/examples/uds.rs

console-subscriber/src/builder.rs

hawkw · 2022-12-16T17:40:43Z

console-subscriber/src/builder.rs

@@ -138,7 +140,7 @@ impl Builder {
    /// defaults.


this documentation should be updated to state that the provided address can be a TCP address, or (on Unix systems) a UDS address. It would be nice to show examples of both usages in this method's documentation.

console-subscriber/src/builder.rs

hawkw · 2022-12-16T19:26:41Z

It looks like, because additional documentation was added to the CLI help text, the documentation needs to be regenerated (which is why CI is failing): https://github.com/tokio-rs/console/actions/runs/3715257647/jobs/6300201144#step:8:150

benesch

Awesome, thanks for the second pass! Should be ready to go.

benesch · 2022-12-16T21:30:04Z

console-subscriber/src/builder.rs

@@ -138,7 +140,7 @@ impl Builder {
    /// defaults.


console-subscriber/src/builder.rs

console-subscriber/examples/uds.rs

benesch · 2022-12-27T17:16:46Z

Sorry to be a bother, @hawkw, but could you trigger a CI run?

hawkw · 2022-12-27T20:25:38Z

Sorry to be a bother, @hawkw, but could you trigger a CI run?

Yup, sorry, I hadn't realized you pushed more commits. Thanks for the reminder!

hawkw

This looks great! Thanks again for working on it!

I'm going to merge this as soon as CI passes. :)

Add support for console connections that use Unix domain sockets rather than TCP. Fix tokio-rs#296.

benesch · 2022-12-28T04:44:45Z

Thank you! I fixed the UDS example not compiling on Windows with some conditional compilation. Didn't see a better way. There's no equivalent of required-features for an example (e.g., required-platform) in Cargo.toml.

Also looks like there's a clippy failure that's unrelated to this PR. Want me to push up a fix in this PR, or would you rather address separately?

benesch · 2023-01-07T04:38:00Z

Also looks like there's a clippy failure that's unrelated to this PR. Want me to push up a fix in this PR, or would you rather address separately?

Gentle ping on this question, @hawkw!

console-subscriber/src/builder.rs

benesch · 2023-03-29T05:26:11Z

Thanks, Eliza! Excited to have this merged.

Add support for console connections that use Unix domain sockets rather than TCP. Closes #296. Co-authored-by: Eliza Weisman <eliza@buoyant.io>

# Changelog All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## console-subscriber-v0.2.0 - (2023-09-29) [0b0c1af](https://github.com/tokio-rs/console/commit/0b0c1aff18c3260d3a45a78f6c0d6f4206af1cbb)...[0b0c1af](https://github.com/tokio-rs/console/commit/0b0c1aff18c3260d3a45a78f6c0d6f4206af1cbb) ### <a id = "console-subscriber-v0.2.0-breaking"></a>Breaking Changes - **Update Tonic and Prost dependencies ([#364](#364 ([f9b8e03](https://github.com/tokio-rs/console/commit/f9b8e03bd7ee1d0edb441c94a93a350d5b06ed3b))<br />This commit updates the public dependencies `prost` and `tonic` to semver-incompatible versions (v0.11.0 and v0.8.0, respectively). This is a breaking change for users who are integrating the `console-api` protos with their own `tonic` servers or clients. - **Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464 ([96e62c8](https://github.com/tokio-rs/console/commit/96e62c83ef959569bb062dc8fee98fa2b2461e8d))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.9.x and `prost` 0.11.x. ### Added - [**breaking**](#console-subscriber-v0.2.0-breaking) Update Tonic and Prost dependencies ([#364](#364)) ([f9b8e03](f9b8e03)) - Add support for Unix domain sockets ([#388](#388)) ([a944dbc](a944dbc), closes [#296](#296)) - Add scheduled time per task ([#406](#406)) ([f280df9](f280df9)) - Add task scheduled times histogram ([#409](#409)) ([d92a399](d92a399)) - Update `tonic` to 0.9 ([#420](#420)) ([48af1ee](48af1ee)) - Update MSRV to Rust 1.60.0 ([b18ee47](b18ee47)) - Expose server parts ([#451](#451)) ([e51ac5a](e51ac5a)) - Add cfg `console_without_tokio_unstable` ([#446](#446)) ([7ed6673](7ed6673)) - Add warning for tasks that never yield ([#439](#439)) ([d05fa9e](d05fa9e)) - [**breaking**](#console-subscriber-v0.2.0-breaking) Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464)) ([96e62c8](96e62c8)) ### Documented - Fix unclosed code block ([#463](#463)) ([362bdbe](362bdbe)) - Update MSRV version docs to 1.64 ([#467](#467)) ([94a5a51](94a5a51)) ### Fixed - Fix build on tokio 1.21.0 ([#374](#374)) ([c34ac2d](c34ac2d)) - Fix off-by-one indexing for `callsites` ([#391](#391)) ([43891ab](43891ab)) - Bump minimum Tokio version ([#397](#397)) ([bbb8f25](bbb8f25), fixes [#386](#386)) - Fix self wakes count ([#430](#430)) ([d308935](d308935)) - Remove clock skew warning in `start_poll` ([#434](#434)) ([4a88b28](4a88b28)) - Do not report excessive polling ([#378](#378)) ([#440](#440)) ([8b483bf](8b483bf), closes [#378](#378)) - Correct retain logic ([#447](#447)) ([36ffc51](36ffc51)) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

# Changelog All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## tokio-console-v0.1.10 - (2023-09-29) [7d009f8](https://github.com/tokio-rs/console/commit/7d009f87120ce0c89f5f9c5311f05b6756ca770f)...[7d009f8](https://github.com/tokio-rs/console/commit/7d009f87120ce0c89f5f9c5311f05b6756ca770f) ### <a id = "tokio-console-v0.1.10-breaking"></a>Breaking Changes - **Update Tonic and Prost dependencies ([#364](#364 ([f9b8e03](https://github.com/tokio-rs/console/commit/f9b8e03bd7ee1d0edb441c94a93a350d5b06ed3b))<br />This commit updates the public dependencies `prost` and `tonic` to semver-incompatible versions (v0.11.0 and v0.8.0, respectively). This is a breaking change for users who are integrating the `console-api` protos with their own `tonic` servers or clients. - **Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464 ([96e62c8](https://github.com/tokio-rs/console/commit/96e62c83ef959569bb062dc8fee98fa2b2461e8d))<br />This is a breaking change for users of `console-api` and `console-subscriber`, as it changes the public `tonic` dependency to a semver-incompatible version. This breaks compatibility with `tonic` 0.9.x and `prost` 0.11.x. ### Added - [**breaking**](#tokio-console-v0.1.10-breaking) Update Tonic and Prost dependencies ([#364](#364)) ([f9b8e03](f9b8e03)) - Only suggest opening issues for panics ([#365](#365)) ([da2a89c](da2a89c)) - Init error handling before subcmds ([#365](#365)) ([ec66eda](ec66eda)) - Filter out boring frames in backtraces ([#365](#365)) ([95a5e54](95a5e54)) - Include config options in autogenerated issues ([#365](#365)) ([3244a1f](3244a1f)) - Reduce decimal digits in UI ([#402](#402)) ([c13085e](c13085e)) - Use tokio task ids in task views ([#403](#403)) ([f5b06d2](f5b06d2)) - Add support for Unix domain sockets ([#388](#388)) ([a944dbc](a944dbc), closes [#296](#296)) - Add scheduled time per task ([#406](#406)) ([f280df9](f280df9)) - Add task scheduled times histogram ([#409](#409)) ([d92a399](d92a399)) - Update `tonic` to 0.9 ([#420](#420)) ([48af1ee](48af1ee)) - Update MSRV to Rust 1.60.0 ([b18ee47](b18ee47)) - Migrate to `ratatui` and update `crossterm` ([#425](#425)) ([b209dd6](b209dd6)) - Help view modal ([#432](#432)) ([359a4e7](359a4e7)) - Add way to inspect details of task from resource view ([#449](#449)) ([132ed4e](132ed4e), closes [#448](#448)) - Add warning for tasks that never yield ([#439](#439)) ([d05fa9e](d05fa9e)) - [**breaking**](#tokio-console-v0.1.10-breaking) Update `tonic` to v0.10 and increase MSRV to 1.64 ([#464](#464)) ([96e62c8](96e62c8)) ### Documented - Update screenshots in README ([#419](#419)) ([e9bcd67](e9bcd67)) - Revert "update screenshots in README ([#419](#419))" ([993a3d9](993a3d9)) - Update screenshots in README ([#421](#421)) ([8a27f96](8a27f96)) - Add column descriptions for all tables ([#431](#431)) ([e3cf82b](e3cf82b)) - Update MSRV version docs to 1.64 ([#467](#467)) ([94a5a51](94a5a51)) ### Fixed - Fix ascii-only flipped input ([#377](#377)) ([652ac34](652ac34)) - Declare `tokio-console` bin as `default-run` ([#379](#379)) ([9ce60ec](9ce60ec)) - Make `retain_for` default to 6s if not specfied ([#383](#383)) ([0a6012b](0a6012b), fixes [#382](#382)) - Enable view-switching keystrokes on details views ([#387](#387)) ([f417d7a](f417d7a)) - Fix `ViewOptions` default lang' ([#394](#394)) ([a1cf1b8](a1cf1b8), fixes [#393](#393)) - Remove `tracing-subscriber` 0.2 from dependencies ([#404](#404)) ([768534a](768534a)) - Fix calculation of busy time during poll ([#405](#405)) ([e2c536a](e2c536a)) - Remove histogram minimum count ([#424](#424)) ([02cf8a6](02cf8a6)) - Remove trailing space from task/resource location ([#443](#443)) ([90e5918](90e5918)) - Make long locations readable ([#441](#441)) ([9428d7f](9428d7f), closes [#411](#411)) - Fix task detail view Id to display remote tokio::task::Id ([#455](#455)) ([70c3952](70c3952)) Signed-off-by: Eliza Weisman <eliza@buoyant.io>

benesch requested a review from a team as a code owner December 4, 2022 13:15

benesch commented Dec 4, 2022

View reviewed changes

hawkw requested changes Dec 6, 2022

View reviewed changes

teskje mentioned this pull request Dec 9, 2022

Support UDS listen address for tokio-console MaterializeInc/materialize#16537

Merged

3 tasks

benesch force-pushed the uds branch 2 times, most recently from 862cc4b to d93e724 Compare December 11, 2022 16:08

benesch commented Dec 11, 2022

View reviewed changes

benesch force-pushed the uds branch 2 times, most recently from 560e0c4 to bac69ec Compare December 11, 2022 20:32

hawkw approved these changes Dec 16, 2022

View reviewed changes

benesch force-pushed the uds branch from fda47be to 661756b Compare December 16, 2022 21:36

benesch commented Dec 16, 2022

View reviewed changes

hawkw approved these changes Dec 27, 2022

View reviewed changes

hawkw enabled auto-merge (squash) December 27, 2022 20:26

feat(console): add support for Unix domain sockets

19ce422

Add support for console connections that use Unix domain sockets rather than TCP. Fix tokio-rs#296.

auto-merge was automatically disabled December 28, 2022 04:43
Head branch was pushed to by a user without write access

benesch force-pushed the uds branch from 661756b to 19ce422 Compare December 28, 2022 04:43

Merge branch 'main' into uds

d4f168f

hawkw enabled auto-merge (squash) February 6, 2023 18:46

Merge branch 'main' into uds

196e31f

hawkw reviewed Mar 28, 2023

View reviewed changes

console-subscriber/src/builder.rs Outdated Show resolved Hide resolved

fix doctest on windows

813951a

hawkw merged commit bff8b8a into tokio-rs:main Mar 28, 2023

hawkw added a commit that referenced this pull request Sep 29, 2023

feat(console): add support for Unix domain sockets (#388)

a944dbc

Add support for console connections that use Unix domain sockets rather than TCP. Closes #296. Co-authored-by: Eliza Weisman <eliza@buoyant.io>

	/// The address of a console-enabled process to connect to.
	///
	/// This may be an IP address and port, or a DNS name.
	///
	/// [default: http://127.0.0.1:6669]

feat(console): add support for Unix domain sockets #388

feat(console): add support for Unix domain sockets #388

Conversation

benesch commented Dec 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw 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

benesch 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

benesch commented Dec 16, 2022

hawkw commented Dec 16, 2022

benesch commented Dec 16, 2022

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Dec 16, 2022

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benesch commented Dec 27, 2022

hawkw commented Dec 27, 2022

hawkw left a comment

Choose a reason for hiding this comment

benesch commented Dec 28, 2022

benesch commented Jan 7, 2023

benesch commented Mar 29, 2023