[RFC] make Telemetry UserID be a uuid string #574
Conversation
|
Current dependencies on/for this PR: This comment was auto-generated by Graphite. |
internal/telemetry/segment.go
Outdated
| } | ||
|
|
||
| hash := hashFromUsername(username) | ||
| uid, err := uuid.FromBytes([]byte(hash)[:16]) |
There was a problem hiding this comment.
I don't think this will create a UUID with a valid version/variant since all of the bytes are random. This might not matter for Sentry though (I'm not sure why they want a UUID specifically). uuid.NewHash might be a better way of hashing the username into a UUID.
There was a problem hiding this comment.
Segment (not sentry) doesn't ask for a UUID. They do recommend choosing a UserID that will be stable for the same user, so we can track the same user over time.
Prior to this PR, I was using just the raw 64-char hash. But I felt that would be too large, and unstructured in case we choose to use that value later on in some internal database (hypothetically). So, I am exploring using a uuid type instead.
Thanks for pointing out the problem about the invalid version/variant. Will look into that.
internal/telemetry/segment.go
Outdated
| const githubPrefix = "github:" | ||
| mac.Write([]byte(githubPrefix + username)) | ||
|
|
||
| return hex.EncodeToString(mac.Sum(nil)) |
There was a problem hiding this comment.
You could not encode the hash to hex and use the byte slice directly.
savil
force-pushed
the
savil/truncate-user-id
branch
from
5aa722b to
9e19455
Compare
Summary
Motivation:
From inspecting the segment logs, I am reminded that the userID is logged as a 64 char.
Segment says that ideally this userID is something we track in our main database. Longer term, I'm not sure we'd want our primary userIDs to be TEXT kind in Postgres data type terms, compared to smaller uuid or int variations.
Approach:
This PR amends the
UserIDFromGithubUsernamefunction to deterministicallyreturn a UUID string (or empty string if not authenticated or error occurs).
It is important that the same UUID is returned for the same github username.
RFC: Breaking Change
Tradeoff: in Segment we'll not be able to track the same users' activity prior to this PR.
How was it tested?
copied this function into a devbox-project for golang and executed it multiple times
with different usernames.