[telemetry] update event name and change is_cloud to shell_access kind #562
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
| AnonymousId: evt.AnonymousID, | ||
| Event: evt.eventName, | ||
| // Event name. We trim the prefix from shell-interactive/shell-ready to avoid redundancy. | ||
| Event: fmt.Sprintf("[%s] Shell Event: %s", evt.AppName, strings.TrimPrefix(eventName, "shell-")), |
There was a problem hiding this comment.
Are you sure this is what we want?
Wouldn't this add "Shell Event" to all events including the "Command" events that already exist?
I would have thought instead that you need to change the string wherever the shell-interactive event is triggered.
There was a problem hiding this comment.
no, those command events are still logged in midcobra/telemetry.go.
I have a cleanup I want to do to unify these further, but have held back since its a bit involved.
internal/telemetry/segment.go
Outdated
| } | ||
|
|
||
| func shellAccess() shellAccessKind { | ||
| if os.Getenv("START_WEB_TERMINAL") == "1" { |
There was a problem hiding this comment.
OK, but ... Could you add a TODO/NOTE of some kind? At some point we'll have VMs with Gotty started that can be accessed via ssh or web, and this would treat them all as web. Really what we care about is the connection, that is running the command, not whether the VM has Gotty running or not.
There was a problem hiding this comment.
sure I can add a comment...
There was a problem hiding this comment.
If you look at how START_WEB_TERMINAL works, its purpose is to ensure that we start the VM for browser shells. Whenever that logic changes, we'll need to update this as well.
@LucilleH suggested using this. Is there another more persistent way to detect that the shell is running as a browser shell?
There was a problem hiding this comment.
I think this is the right behavior for now – I'm just saying we'll need to improve it and I like adding a TODO so it doesn't slip our minds.
There was a problem hiding this comment.
The other approach is to rely on an SSH env var like SSH_USER_PUBLIC_KEY so:
// Check if in cloud
if os.Getenv("DEVBOX_REGION") != "" {
if os.Getenv("SSH_USER_PUBLIC_KEY") != "" {
return ssh
} else {
return browser
}
}
return local
(need to test this)
There was a problem hiding this comment.
I can use SSH_TTY
I dumped env for both ssh and browser shells to compare:
❯ cat web_env.txt | grep SSH
SSH_HOST_KEY=
DEFAULT_SSH_USER=devbox
SSH_USER=savil
SSH_USER_PUBLIC_KEY=
SSH_LISTEN_IPV4=0.0.0.0
SSH_LISTEN_IPV6=::
SSH_PORT=2222
❯ cat ssh_env.txt | grep SSH
SSH_CONNECTION=fdaa:0:a780:a7b:b2e2:41a9:b82f:2 58826 fdaa:0:a780:a7b:a1:ef3c:55b5:2 2222
SSH_CLIENT=fdaa:0:a780:a7b:b2e2:41a9:b82f:2 58826 2222
SSH_TTY=/dev/pts/0
There was a problem hiding this comment.
I think for now this is fine to use START_WEB_TERMINAL 👍
internal/telemetry/segment.go
Outdated
| } | ||
|
|
||
| func shellAccess() shellAccessKind { | ||
| if os.Getenv("START_WEB_TERMINAL") == "1" { |
There was a problem hiding this comment.
I think this is the right behavior for now – I'm just saying we'll need to improve it and I like adding a TODO so it doesn't slip our minds.
savil
force-pushed
the
savil/shell-tti-events
branch
from
b2ff48a to
11c4322
Compare
savil
force-pushed
the
savil/shell-tti-events
branch
from
11c4322 to
0af9aac
Compare
internal/telemetry/segment.go
Outdated
| } | ||
|
|
||
| func shellAccess() shellAccessKind { | ||
| if os.Getenv("START_WEB_TERMINAL") == "1" { |
There was a problem hiding this comment.
I think for now this is fine to use START_WEB_TERMINAL 👍
| // Check if running in devbox cloud | ||
| if os.Getenv("DEVBOX_REGION") != "" { | ||
| // Check if running via ssh tty (i.e. ssh shell) | ||
| if os.Getenv("SSH_TTY") != "" { |
There was a problem hiding this comment.
@loreto I think this approach is closer to what you wanted where we test for ssh versus browser based on access methodology.
There was a problem hiding this comment.
Sounds good! Just make sure you test it :)
There was a problem hiding this comment.
I verified that SSH_TTY isn't present in browser shell:
#562 (comment)
Summary
Updating:
[devbox] Shell event: interactivewhich is consistent with the command event names.local | ssh | browserHow was it tested?
local: run localdevbox shelland verify event logged in segment dashboard.ssh: deploy custom VM with custom devbox binary. verify event logged in segment dashboard.browser: Follow devbox-cloud README for http-gateway, but how do I test the custom devbox binary?