Skip to content

chore(vpn): send ping results over tunnel #18200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jun 6, 2025
Merged

Conversation

ethanndickson
Copy link
Member

@ethanndickson ethanndickson commented Jun 3, 2025

Closes #17982.

The purpose of this PR is to expose network latency via the API used by Coder Desktop.

This PR has the tunnel ping all known agents every 5 seconds, in order to produce an instance of:

message LastPing {
	// latency is the RTT of the ping to the agent.
	google.protobuf.Duration latency = 1;
	// did_p2p indicates whether the ping was sent P2P, or over DERP.
	bool did_p2p = 2;
	// preferred_derp is the human readable name of the preferred DERP region,
	// or the region used for the last ping, if it was sent over DERP.
	string preferred_derp = 3;
	// preferred_derp_latency is the last known latency to the preferred DERP
	// region. Unset if the region does not appear in the DERP map.
	optional google.protobuf.Duration preferred_derp_latency = 4;
}

The contents of this message are stored and included on all subsequent upsertions of the agent.
Note that we upsert existing agents every 5 seconds to update the last_handshake value.

On the desktop apps, this message will be used to produce a tooltip similar to that of the VS Code extension:
image
(wording not final)

Unlike the VS Code extension, we omit:

  • The Latency of all available DERP regions. It seems not ideal to send a copy of this entire map for every online agent, and it certainly doesn't make sense for it to be on the Agent or LastPing message.
    If we do want to expose this info on Coder Desktop, we should consider how best to do so; maybe we want to include it on a more generic Netcheck message.
  • The current throughput (Bytes up/down). This is out of scope of the linked issue, and is non-trivial to implement. I'm also not sure of the value given the frequency we're doing these status updates (every 5 seconds).
    If we want to expose it, it'll be in a separate PR.
image

Copy link
Member Author

ethanndickson commented Jun 3, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for sending ping results over the tunnel by adding a new LastPing proto message, updating version numbers to 1.2, and refactoring agent structures and latency recording logic. Key changes include:

  • Adding the new LastPing message in vpn.proto.
  • Updating version information in vpn/version.go and related tests.
  • Refactoring agent types to include ping data and implementing latency recording in tunnel.go.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
vpn/vpn.proto Added LastPing message definition and field in Agent message
vpn/version.go Updated version from 1.1 to 1.2 with new network-related information
vpn/tunnel_internal_test.go Updated tests to verify ping and latency data in agent updates
vpn/tunnel.go Refactored agent types, updated conversion functions, and added latency recording
vpn/speaker_internal_test.go Updated expected handshake version
vpn/client.go Added Ping, Node, and DERPMap functions to the VPN connection
tailnet/derpmap_test.go & derpmap.go Added functions to extract DERP latency and preferred DERP names
coderd/util/maps/maps.go Added a generic Map function for maps
coderd/database/db2sdk/db2sdk.go Removed duplicate Map function
cli/ssh.go Updated DERP latency extraction to use new tailnet functions

@ethanndickson ethanndickson marked this pull request as ready for review June 3, 2025 06:50
johnstcn
johnstcn previously approved these changes Jun 3, 2025
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring approval to netgru folks, but I don't see any blocking issues 👍 I like the agent pings, this could potentially give us some very useful information!

@ethanndickson ethanndickson marked this pull request as draft June 4, 2025 11:15
@ethanndickson
Copy link
Member Author

Found some issues, don't review.

deansheather
deansheather previously approved these changes Jun 5, 2025
@deansheather deansheather dismissed stale reviews from johnstcn and themself June 5, 2025 03:13

still draft

@ethanndickson ethanndickson marked this pull request as ready for review June 5, 2025 03:15
Copy link
Member Author

ethanndickson commented Jun 6, 2025

Merge activity

  • Jun 6, 4:18 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jun 6, 4:18 AM UTC: @ethanndickson merged this pull request with Graphite.

@ethanndickson ethanndickson merged commit 0076e84 into main Jun 6, 2025
35 checks passed
@ethanndickson ethanndickson deleted the ethan/send-ping-results branch June 6, 2025 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support exposing workspace latency for Coder Connect
3 participants