- Notifications
You must be signed in to change notification settings - Fork905
chore(vpn): send info, debug logs over tunnel#18240
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ethanndickson commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
@@ -65,7 +65,6 @@ func (r *RootCmd) vpnDaemonRun() *serpent.Command { | |||
logger.Info(ctx, "starting tunnel") | |||
tunnel, err := vpn.NewTunnel(ctx, logger, pipe, vpn.NewClient(), | |||
vpn.UseOSNetworkingStack(), | |||
vpn.UseAsLogger(), |
ethanndicksonJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
The Windows app drops logs received over the tunnel, so this can be removed. It adds it's own sink tostderr
.
select { | ||
case <-t.updater.ctx.Done(): | ||
return | ||
case <-t.ctx.Done(): | ||
return | ||
case t.sendCh <- msg: |
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.
We deliberately never cancel the tunnel ctx, due to the somewhat precarious teardown procedure. We do however cancel the updater ctx to ensure we don't tick network updates after shutdown, so we'll check for that here too.
logMu sync.Mutex | ||
logs []*TunnelMessage |
ethanndicksonJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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'm not seeing any value in this buffer, as we don't do any disk I/O - the pipe is entirely in-memory. With pings every 5 seconds and debug logs on, we produce around 150~ logs a minute, or about 35 kilobytes. The in-memory pipe can handlegigabytes a second. I think buffering would make sense were we relaying the logs to a terminal or a file, but that's not the case here.
Since only Error and above logs are flushed immediately, the alternative is adding the complexity of flushing the logs from the buffer periodically. One downside of this is that the macOS timestamps on the logs once they reach the Swift code aren't actually representative of the time the log was produced.
c65ebb2
to8d8937d
Compare8d8937d
toc5c1ab5
Compareethanndickson commentedJun 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Merge activity
|
c5c1ab5
to8535d64
Compareethanndickson commentedJun 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Review request can be ignored - graphite touched proto files for a moment doing it's branch switching + rebase (why?). |
628b81c
intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closescoder/internal#397