GeezerVison

Could make sense to also rename the column to app_name? Type to me implies a grouping like we had before, but now these are individual app names.

Although for web apps it is still a grouping. 🤔 I suppose we could put the slug here instead? But I see we already have slug_or_port.

And we still have types like tunnel. hrmm. Maybe we need both type and app_name? For apps like VS Code we could set the type to ssh? Then we could use slug_or_port to store the app name vscode? (possibly renaming it to app_name_or_port)

Actually true, this was a bit inconsistent and confusing. type was mixing apps with connection types. Went with your app_name_or_port idea, plus a rename of type:

• type is now kind, a closed enum of what observed the connection: the agent for ssh and reconnecting_pty, coderd for workspace_app, port_forwarding, and tunnel. VS Code, Cursor, and Zed are all ssh, as you suggested.
• slug_or_port is now app_name_or_port, read by kind: the app an agent reported, a workspace app slug, or a port.

Both are renames only, so the table is no longer rewritten and the enum stays. Families are never stored: the API type is the grouping again, derived from the app registry on read, so Cursor returns type: vscode with app_name: cursor, and adding an app or family needs no migration. Older rows keep vscode/jetbrains in kind with no app name, and read as that app. There's also a new app: filter that matches an agent app name or a workspace app slug.

Awesome!!

I think we should replace vscode and jetbrains now in the migration with ssh (and populate their app_name_or_port with vscode or jetbrains as appropriate) to match the data going forward.

And then, if we do that, the "kind" name might need changing. The comment says "what observed the connection" which makes me think "subsystem" although maybe it is more like "transport" (or dare I say "connection type"). Although transport means something specific so maybe subsystem would be better.

Like the data goes through SSH, or a workspace app, or a port forward, or the agent's reconnecting-pty HTTP API, many of which flow through (and are observed, if not reported by) both coderd and the agent.

And then, where we report the connection, instead of mapping the app name to the transport (like mapping jetbrains/vscode to SSH), we should just rely on what we know the actual subsystem is (so for agentssh.NewServer obviously we know it is always ssh, etc).

RFC says to get rid of the enum and we should! The values that exist don't cover all the possibilities now or in future, and lead to us skipping connection logs (if the kind is unknown) or writing something not true (like assuming anything we don't recognize is SSH).

Okay I've replaced the connection type with with source which is a string with 4 known values as we agreed above, by grouping ssh and reconnecting_pty it actually made the queries and logic everywhere simpler!

I'll now run a more comprehensive benchmark and see if this table is unbounded and how many rows might there be.

Edit: Added the synthetic benchmarks in the description.

Yeah I think the problem is that we had the connection "type" tangled up with the name of the app making that connection. App names are dynamic; new ones can appear at any time, requiring coderd source change to start tracking them, which was the problem.

Connection types are static, the only way new types appear is if someone modifies the source. Since the source is being modified, the enum can be modified at the same time. And since the types are well-known and static, an enum seems a natural fit to me.

I think at the time of the RFC we were thinking the type column was essentially just a static "app name", or at least that is how I was thinking about it, and so that we could therefore drop it (and its enum) entirely.

We could still drop type and its enum entirely, if we are not particularly concerned with how the connection was made or what subsystem it went through, just that it exists and what app it was for, and fall back to the "type" for the app name when the app name is unknown (so an SSH connection would show "ssh" for the app name instead of "ssh" for the type and a blank app name).

But that would also collapse some things into each other, like web VS Code versus desktop VS Code would look the same instead of one type being "workspace_app" and another "ssh", both with the "vscode" app name.

Also perhaps interesting to note that even the reconnecting pty can be used by different applications; currently it is used by the web terminal but can also be used on the command line via the cli.

I do think that we should still do string over an enum because we do not know if this list is truly exhaustive. We can do the check in Go but the DB type can be string. Now for agent vs. ssh and reconnecting_pty, if we want to differentiate between say normal CLI (ssh) and reconnecting_pty CLI then we should have the following sources (we can call it protocol now I suppose): ssh, reconnecting_pty, workspace_app, port_forwarding, tunnel, unknown with the following semantics:

┌──────────────────┬──────────────────┬────────────────────┐
│     Old type     │   → protocol     │ → app_name_or_port │
├──────────────────┼──────────────────┼────────────────────┤
│ ssh              │ ssh              │ NULL               │
├──────────────────┼──────────────────┼────────────────────┤
│ vscode           │ ssh              │ vscode             │
├──────────────────┼──────────────────┼────────────────────┤
│ jetbrains        │ ssh              │ jetbrains          │
├──────────────────┼──────────────────┼────────────────────┤
│ reconnecting_pty │ reconnecting_pty │ NULL               │
├──────────────────┼──────────────────┼────────────────────┤
│ web types        │ unchanged        │ unchanged          │
└──────────────────┴──────────────────┴────────────────────┘

In this case, we support proper protocol and we can even expose it in the API (for example, if we wanna tell reconnecting_pty from the web terminal vs. the CLI).

Actually, in this case, the agent.proto type: enum would still be deprecated but we can add a protocol: string so let's resolve this here before we modify #29675...

@spikecurtis @code-asher Thoughts?

In considering whether to have the DB type be enum or string, I think the right question to think about is how important is constraining the set of values? If you do an enum, it's constrained at the database and we literally cannot represent an errant value.

If we do a string, then we lose strong constraints, but gain a lot of flexibility. It's easier to add new things.

For this case, where we are doing a sort of best-effort log of connections, I don't think we gain that much by making the constraints air-tight, and the flexibility to add new things without a DB schema migration are appealing.