Add latency detector for desktop sessions by probakowski · Pull Request #52827 · gravitational/teleport

probakowski · 2025-03-05T23:24:32Z

This PR adds latency measurement for Windows Desktop session by calculating latency from browser to proxy and from proxy to the desktop.

op.

changelog: Add latency detector for desktop sessions

Branched off of #40417

…y-detector

zmb3

We also need to consider compatibility in both directions:

If proxy is ahead of agents, and proxy knows about the new messages but agents don't, then the proxy should either hide latency detector completely, or show only the first leg of the connection. (We can ask design for ideas how to indicate this in the UI).
If agents are ahead of proxy, then the proxy should just drop latency messages and not crash on an unknown message type.

Please try both use cases and let me know what your plan for addressing them is.

…-latency-detector

This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear.

gzdunek · 2025-03-19T14:10:17Z

I pushed a fix for the tooltip not disappearing when the popup got closed. The problem occurred because the tooltip was applied to the entire popup instead of just the icon. I suspect the modal, which was the child of the tooltip, prevented its onMouseLeave handler from being triggered.
Applying a tooltip to a menu feels incorrect anyways, so I moved it to the icon instead.

…-latency-detector

probakowski · 2025-03-26T23:46:36Z

@zmb3 I've tested backward compatibility in both directions

if proxy is ahead of WDS I've added version check and we won't send Ping messages avoiding error, we won't show the latency detector UI at all in such case
if WDS is ahead of proxy it will never send Ping message as well because it does it only in response to the message received from proxy

…-latency-detector

zmb3

LGTM other than the v18/v17.5 decision.

Also, please update the RFD with all the TDP messages to account for the new latency stats and ping messages.

backport-bot-workflows · 2025-05-07T08:48:20Z

@probakowski See the table below for backport results.

Branch	Result
branch/v17	Failed

* wip * add ping message and latency from desktop side * version * Fix backward compatibility * godocs * formatting * fix imports * formatting * formatting * log and gci * lint * Apply tooltip on the icon directly, instead of on the Menu component This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear. * Use consistent spacing between top bar elements * updates from origin * e * e * lint * rework UI after merge * Rename fields in backend * prettier * Update web/packages/shared/components/DesktopSession/TopBar.tsx Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> * review comments * fix ui * add env var to disable windows desktop "ping" * review comment * review comment * Update lib/web/desktop.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Refactor latency monitoring * fix spelling * remove monitorSessionLatency * gci * review comment * review comment * Update RFD and version * fix gaps --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com>

espadolini · 2025-05-08T15:13:20Z

+	// run a goroutine to pick TDP messages up from a channel and send
+	// them to the browser
+	errs.Go(func() error {
+		for msg := range tdpMessagesToSend {


Does this not just block forever and leak the goroutine at the end of a connection? Nothing closes tdpMessagesToSend.

It does, could you please check 0551d44 where I tried to fix all 3 problems you mention

espadolini · 2025-05-08T15:21:21Z

+	errs.Go(func() error {
+		for msg := range tdpMessagesToSend {
+			if ping, ok := msg.(tdp.Ping); ok {
+				pings <- ping


This can also block if the context for monitorLatency is canceled and nothing reads from the pings channel anymore.

espadolini · 2025-05-08T15:24:31Z

+			if !disableDesktopPing {
+				conn, err := net.Dial("tcp", c.cfg.Addr)
+				if err == nil {
+					conn.Close()
+				}
+			}


This will just block the whole stream of messages until the dial is completed on every ping, won't it?

espadolini · 2025-05-08T15:29:00Z

-	errs := make(chan error, 2)
+	tdpMessagesToSend := make(chan tdp.Message)
+
+	latencySupported, err := utils.MinVerWithoutPreRelease(version, "17.5.0")


Instead of relying on indirect info such as the version string (in a heartbeat, even, not even from the actual connection in use) can we start thinking about exchanging capabilities as part of the TDP "handshake", so to speak?

* wip * add ping message and latency from desktop side * version * Fix backward compatibility * godocs * formatting * fix imports * formatting * formatting * log and gci * lint * Apply tooltip on the icon directly, instead of on the Menu component This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear. * Use consistent spacing between top bar elements * updates from origin * e * e * lint * rework UI after merge * Rename fields in backend * prettier * Update web/packages/shared/components/DesktopSession/TopBar.tsx Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> * review comments * fix ui * add env var to disable windows desktop "ping" * review comment * review comment * Update lib/web/desktop.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Refactor latency monitoring * fix spelling * remove monitorSessionLatency * gci * review comment * review comment * Update RFD and version * fix gaps --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> (cherry picked from commit 1aba870)

…54926) * Show Windows desktops in Connect (#53955) * Show Windows desktops in Connect * Use `net.SplitHostPort` * Use underscore in URIs * Omit the default RDP port before displaying item in the UI (cherry picked from commit 81af813) * Add desktop session-related ACLs to Connect (#54031) * Add desktop session-related ACLs to Connect * Add new fields to `makeAcl` (cherry picked from commit aba0de0) * Add latency detector for desktop sessions (#52827) * wip * add ping message and latency from desktop side * version * Fix backward compatibility * godocs * formatting * fix imports * formatting * formatting * log and gci * lint * Apply tooltip on the icon directly, instead of on the Menu component This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear. * Use consistent spacing between top bar elements * updates from origin * e * e * lint * rework UI after merge * Rename fields in backend * prettier * Update web/packages/shared/components/DesktopSession/TopBar.tsx Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> * review comments * fix ui * add env var to disable windows desktop "ping" * review comment * review comment * Update lib/web/desktop.go Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> * Refactor latency monitoring * fix spelling * remove monitorSessionLatency * gci * review comment * review comment * Update RFD and version * fix gaps --------- Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com> Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> (cherry picked from commit 1aba870) * Abstract directory sharing file system (#54545) * Extract an interface to interact with a shared directory file system Some methods were renamed to more closely follow common file system naming conventions. * Use the new interface in the TDP client * Provide a browser file system as the shared directory access implementation (cherry picked from commit d3fbeeb) * Support desktop access in Connect (#54373) * Extract reusable function for establishing connections to Windows Desktop Service * Add `ProxyWindowsDesktopSession` proto * Implement `ProxyWindowsDesktopSession` * Enable fetching desktops and desktop services in remote proxy cache * Implement dialing windows desktop * Implement client * Support Windows desktop certs in tsh * Fix incorrect `windowsDesktop` URI * Add proto for `ConnectToDesktop` * Implement `ConnectToDesktop` * Do not log requests/responses for `ConnectToDesktop` RPC * Add boilerplate for `DocumentDesktopSession` * Open a desktop connection * Relax ArrayBuffer type passed to encode methods, ignore tshd abort errors In tshd stream, the buffer is of type `ArrayBufferLike` (which is `ArrayBuffer` & `SharedArrayBuffer`). To allow assigning it to the type in our TDP code, we make it more general. * Ensure WASM IronRDP code is initialized only once * Use `utils.ShuffleVisit` * Improve stream cancellation handling * Leave a TODO about ListWindowsDesktops * Do not return empty data slice * Provide non-nil src and dest addresses to `streamutils.NewConn()` * Do not emit an empty message to indicate a successful connection * Fix test * Simplify code * Add missing `WindowsDesktopTLSCredentials` initialization * Require that the first message is only a dial request and the subsequent ones are only data * Add explicit `stop()` check * Hold cluster name and desktop name in a struct for the map key * Do not return early on non-connection problem errors * Handle io.EOF error specifically in BidiStreamingClient.Send instead of in `tlsConn.HandshakeContext` * Extract a common function to proxy TDP connections * Improve proto comments and connection setup * Add comments and logs * Lint * Explain why there's a special handling for abort error * Bring back the original `proxyWebsocketConn` behavior when it comes to error handling * Post merge fixes * Adjust proxying TDP connection to changes from master * Lint * Channels improvements * Post merge fixes (cherry picked from commit 980ce61) * Show desktops in connection tracker (#54668) * Make supporting non-Windows desktops easier in the future * Add connection boilerplate for desktop connections * Show connected/disconnected status in the connection tracker * Make sure that ACLs were fetched before reading from them ACLs are fetched asynchronously. * Do not crash when reading unsupported connection from app_state.json * `gwDoc` -> `doc` * `windowsDesktops` -> `windows_desktops` (cherry picked from commit 9f49376) * Implement shared directory file system in tsh daemon (#54662) * Implement shared directory file system in tshd * Remove `Open` method * Do not use `os.IsNotExist` * Read bytes into `[]byte` parameter * Correctly use `t.Helper()` * Add missing `trace.Wrap` * Rename receivers * Handle errors from `file.Close()` * Use named returns to return close error * Improve error handling (cherry picked from commit b3e9199) * Support directory sharing in Connect (#54663) * Register directory access when starting a desktop session * Add RPC to attach a directory to desktop session * Do not allow `attachDirectoryToDesktopSession` to be called from the renderer process * Open the directory picker and send the selected path to tshd * Intercept file system events coming from the server and handle them * Disallow file system messages to be sent from the renderer * Refactor dir sharing * `AttachDirectoryToDesktopSession` -> `SetSharedDirectoryForDesktopSession` * Improve comments * Small fixes * Add missing defer for `s.dirAccessMu.RUnlock()` * `TestOpenSharedDirectory` -> `TestNewDirectoryAccess` * Add a comment for JS file system handlers * `make grpc` --------- Co-authored-by: Rafał Cieślak <rafal.cieslak@goteleport.com> (cherry picked from commit 45428c0) * Simplify code around latency detection for desktop (#54795) * Simplify code around latency detection for desktop * cleanup * Use tdp.Conn for client and server * Add check for context cancellation * Cleanup * remove context check * remove context check * remove unused field * use sync.OnceFunc, error on unexpected ping * err message (cherry picked from commit 28f78c3) * Show `desktop access requires Teleport Proxy 17.5.0 or higher` when Proxy returns NotImplementedError * Add `wasm-unsafe-eval` to Connect CSP (#54916) (cherry picked from commit ee8be4d) * Ensure all errors thrown by `TdpClient` are instances of `Error` class (#54877) * Add a utility function to convert any input to an Error instance * Add types for TdpClient events * Always throw Error from `adaptWebSocketToTdpTransport` * Improve getting the error message in `getErrMessage` * Move `isAbortError` to error.ts * Remove unused `tshd/errors.ts` * Handle `JSON.stringify` error, add `cause` to thrown errors * Remove unnecessary `async` * Remove `error.toString` from the callsites * Handle `err` being undefined (cherry picked from commit 4d28408) --------- Co-authored-by: Przemko Robakowski <przemko.robakowski@goteleport.com>

zmb3 and others added 3 commits April 10, 2024 08:47

wip

86774cf

Merge remote-tracking branch 'origin/master' into zmb3/desktop-latenc…

f4b581a

…y-detector

add ping message and latency from desktop side

74450eb

probakowski requested a review from zmb3 March 5, 2025 23:24

zmb3 reviewed Mar 6, 2025

View reviewed changes

Comment thread lib/srv/desktop/tdp/proto.go

Comment thread lib/srv/desktop/rdp/rdpclient/client.go

probakowski and others added 12 commits March 14, 2025 21:04

version

fa07c79

Fix backward compatibility

236cbec

godocs

cd45be8

formatting

c0f2dfb

Merge remote-tracking branch 'origin/master' into probakowski/desktop…

78f17d3

…-latency-detector

fix imports

4906593

formatting

8b7ae4b

formatting

c9b6a01

log and gci

d8b7aad

lint

5b08181

Apply tooltip on the icon directly, instead of on the Menu component

88db008

This fixes a problem where onMouseLeave in HoverTooltip wasn't called and the tooltip didn't disappear.

Use consistent spacing between top bar elements

853b91c

probakowski added 6 commits March 24, 2025 20:19

Merge remote-tracking branch 'origin/master' into probakowski/desktop…

25a7ef0

…-latency-detector

updates from origin

71e14d0

Merge remote-tracking branch 'origin/master' into probakowski/desktop…

2b8b31a

…-latency-detector

Merge remote-tracking branch 'origin/master' into probakowski/desktop…

eabea28

…-latency-detector

e

58bd4f3

e

bbc1073

probakowski changed the title ~~add latency detector for desktop sessions~~ Add latency detector for desktop sessions Mar 26, 2025

lint

0a1e727

probakowski marked this pull request as ready for review March 26, 2025 23:46

probakowski requested a review from zmb3 March 26, 2025 23:46

github-actions Bot added the desktop-access label Mar 26, 2025

probakowski added 5 commits April 22, 2025 12:36

Merge remote-tracking branch 'origin/master' into probakowski/desktop…

5942a2b

…-latency-detector

fix spelling

4eb3c0a

Merge branch 'master' into probakowski/desktop-latency-detector

ae258df

remove monitorSessionLatency

bf49275

gci

3a30f62

probakowski requested a review from zmb3 April 22, 2025 20:19

zmb3 reviewed Apr 22, 2025

View reviewed changes

Comment thread lib/web/desktop.go Outdated

Comment thread lib/web/desktop.go Outdated

Comment thread lib/web/desktop.go Outdated

probakowski added 2 commits April 23, 2025 22:45

review comment

da25c99

review comment

b5b4c66

probakowski requested a review from zmb3 April 23, 2025 21:26

probakowski added the backport/branch/v17 label Apr 23, 2025

zmb3 approved these changes May 2, 2025

View reviewed changes

probakowski added 4 commits May 6, 2025 21:12

Update RFD and version

cc98086

Merge branch 'master' into probakowski/desktop-latency-detector

f21aec9

fix gaps

b7edc60

Merge branch 'master' into probakowski/desktop-latency-detector

b5e0f90

probakowski enabled auto-merge May 7, 2025 08:11

probakowski added this pull request to the merge queue May 7, 2025

Merged via the queue into master with commit 1aba870 May 7, 2025
43 checks passed

probakowski deleted the probakowski/desktop-latency-detector branch May 7, 2025 08:47

ravicious mentioned this pull request May 7, 2025

Support desktop access in Connect #54373

Merged

probakowski mentioned this pull request May 7, 2025

[v17] Add latency detector for desktop sessions (#52827) #54604

Closed

espadolini reviewed May 8, 2025

View reviewed changes

gzdunek mentioned this pull request May 19, 2025

[v17] Desktop access in Connect and desktop session latency detector #54926

Merged

Conversation

probakowski commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gzdunek commented Mar 19, 2025

Uh oh!

probakowski commented Mar 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

backport-bot-workflows Bot commented May 7, 2025

Uh oh!

espadolini May 8, 2025

Choose a reason for hiding this comment

Uh oh!

probakowski May 8, 2025

Choose a reason for hiding this comment

Uh oh!

espadolini May 8, 2025

Choose a reason for hiding this comment

Uh oh!

espadolini May 8, 2025

Choose a reason for hiding this comment

Uh oh!

espadolini May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

probakowski commented Mar 5, 2025 •

edited

Loading