As far as I can tell, this situation would only be possible if we completely messed up the view. In this case, many other things would be also broken. However, in this case we not only potentially do a whole bunch of user lookups, we also spam a bunch of log errors.

IMO this should just error loudly so it's very obvious.

As far as I can tell, this situation would only be possible if we completely messed up the view.

Yes it seems that way, though I'm not sure if there's some other edge case where this could happen, in which case dropping the fallback path of queryingGetUserByID would techncially be a breaking change.

Removing the logging (warning level for the "workspace did not have a username set") feels reasonable. I would lean towards keeping the fallback DB query unless we have no requirements around breaking changes for metrics.

I would lean towards keeping the fallback DB query unless we have no requirements around breaking changes for metrics.

My point is, if we mess up the view in such a way thatOwnerUsername is empty, it's very likely that more things would be broken than metrics. Having a non-empty username is a pretty fundamental assumption baked into the codebase right now.

Right, but it could be possible thatOwnerUsername is empty for only a very small subset of workspaces.

Is your suggestion that we error updating the metric as a whole (for all found workspace agents) if we see any empty username, or just for those workspaces which have an empty username (and do not call the GetUserByID query as a fallback)?

I see two possible scenarios here:

1. Some users end up actually having empty usernames (which istheoretically possible based on the database schema). In this case, there would be no benefit from callingGetUserByID.

2. Some error updating the view results in an empty string being returned forowner_username. As far as I can tell, this would break things like application routing, SSH access, and also be distinctly visible in the UI. In this case, we have three options:
   a) Continue submitting metrics with an emptyusername field,
   b) Fall back to theGetUserByID query,
   c) Skip over the user/agent.
   d) Refuse to generate potentially invalid metrics at all, error and exit early.

a) could actually signal the issue more quickly, but at the cost of messed up metrics.
b) would likely correct the issue, but we would suddenly be performing more database queries unexpectedly.
c) would also surface the error in a way
d) might actually be overkill now that I think about it.

Out of curiousity, I decided to see what would happen if a migration messed up the view and pushed#19426. The main takeaway I got from that is that a number of existing tests failed, but -- most notably --coderd/prometheusmetrics andcoderd/workspacestats didn't fail. If this is an area of concern, I'd suggest modifying the existing tests to guard for this so that we catch it.

Out of curiousity, I decided to see what would happen if a migration messed up the view and pushed#19426. The main takeaway I got from that is that a number of existing tests failed, but -- most notably -- coderd/prometheusmetrics and coderd/workspacestats didn't fail. If this is an area of concern, I'd suggest modifying the existing tests to guard for this so that we catch it.

IIUC that's because we would still have theGetUserByID call, so the view doesn't matter, we're taking the workspace owner ID and looking up that user.

I'm not sure I understand your point about Bwould likely correct the issue, but we would suddenly be performing more database queries unexpectedly since we're currently making a DB call for every active workspace.

Unless we want to introduce a potentially breaking change I think B is our only option. Otherwise we can remove the fallback query and go with option C, then workspaces with correct usernames set would still emit agent related metrics.

Can we not just add a foreign key constraint on this field? Seems like an odd omission.
Along with that, a constraint to enforce the minimum length of the username would add a lot more integrity.

This would likely render the rest of this discussion moot as this situation would not be possible.
I do appreciate the defensiveness of this code though, thanks@cstyan 👍

That's a good observation@dannykopping - I might suggest a separate PR though.

Good idea 👍 that PR should block this one.

Not sure I understand the FK constraint suggestion, what would that look like or do for us? The query here just uses a viewworkspaces_expanded, and theownerID on theworkspaces table is already a FK tousers. So I think it's just the constraint to enforce a minimum username length that we need to add?

Oh whoops, I didn't see the FK at the bottom of the file:https://github.com/coder/coder/blob/main/coderd/database/dump.sql#L3325-L3326