-
Notifications
You must be signed in to change notification settings - Fork 5
chore: added latency tooltips on workspaces #134
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
base: main
Are you sure you want to change the base?
Conversation
|
||
// For compatibility with older deployments, we assume that if the | ||
// last ping is null, the agent is healthy. | ||
var isLatencyAcceptable = agent.LastPing != null ? agent.LastPing.Latency.ToTimeSpan() < HealthyPingThreshold : true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check that the behavior of the C# protobuf library does this? They usually just use a zero value when it's not set to be consistent with other protobuf implementations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for reference types it will be null.
App/ViewModels/AgentViewModel.cs
Outdated
|
||
var tooltip = description.ToString().TrimEnd('\n', ' '); | ||
|
||
if (string.IsNullOrEmpty(tooltip)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? When would this happen?
I'm not handling it on mac:
var statusString: String {
switch status {
case .okay, .high_latency:
break
default:
return status.description
}
guard let lastPing else {
// Either:
// - Old coder deployment
// - We haven't received any pings yet
return status.description
}
let highLatencyWarning = status == .high_latency ? "(High latency)" : ""
var str: String
if lastPing.didP2p {
str = """
You're connected peer-to-peer. \(highLatencyWarning)
You ↔ \(lastPing.latency.prettyPrintMs) ↔ \(wsName)
"""
} else {
str = """
You're connected through a DERP relay. \(highLatencyWarning)
We'll switch over to peer-to-peer when available.
Total latency: \(lastPing.latency.prettyPrintMs)
"""
// We're not guranteed to have the preferred DERP latency
if let preferredDerpLatency = lastPing.preferredDerpLatency {
str += "\nYou ↔ \(lastPing.preferredDerp): \(preferredDerpLatency.prettyPrintMs)"
let derpToWorkspaceEstLatency = lastPing.latency - preferredDerpLatency
// We're not guaranteed the preferred derp latency is less than
// the total, as they might have been recorded at slightly
// different times, and we don't want to show a negative value.
if derpToWorkspaceEstLatency > 0 {
str += "\n\(lastPing.preferredDerp) ↔ \(wsName): \(derpToWorkspaceEstLatency.prettyPrintMs)"
}
}
}
str += "\n\nLast handshake: \(lastHandshake?.relativeTimeString ?? "Unknown")"
return str
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you handle it here:
guard let lastPing else {
// Either:
// - Old coder deployment
// - We haven't received any pings yet
return status.description
}
I have now replicated this logic by printing the status.
Fixes: #106