Add Codespace usage metrics

[Jongmassey]

closes opensafely-core/codespaces-initiative#42

[Jongmassey]

Two things outstanding on this:

• generating the required PAT for the opensafely org and adding to the prod environment
• future schema changes to the github_codespaces table will require migrations (unlike the other tables that are dropped and recreated each time). There currently isn't a mechanism for this.

[iaindillingham]

Hopefully the comments are useful and interesting. More generally, please take a moment to rework the commit messages. "How to Write a Git Commit Message" contains sound advice. Consider this example, from this PR:

Codespaces database table definition and metric

Use the creation timestamp as PK as near-impossible clash

Slight misuse of "metric" to store raw records of codespaces

Some questions:

• What does this commit do? (imperative mood)
• Why is the clash "near-impossible"? Why not "impossible"?
• What's the 'Slight misuse of "metric"'? Why are we misusing "metric" at all?
• What's wrong with sentences?

[iaindillingham]

But we do expect an instance of Repo when we see repo, don't we? The required fields of Repo are org, name, team, and created_on. We can populate three of these without effort, can we not? What's stopping us from making Repo.created_on optional?

[Jongmassey]

org and name are trivial. team isn't applicable here. We could get created_on in another trip to the API, or change Repo.from_dict() to not require it.

AFAICT we don't need any of these fields beyond the name

[iaindillingham]

From the data perspective, we don't need these fields. Conceptually, however, we represent repositories as instances of the Repo class. Here, you're introducing another representation of repositories: as instances of the str class.

Imagine returning to this code in the future. Why is PR.repo a Repo and Codespace.repo a str? The comment says something about an endpoint and metrics. Might the endpoint change? Might metrics change? We'd have to follow a path through the system to better understand the comment, especially if the endpoint changed.

[Jongmassey]

I've added a commit that switches this to using the Repo type and fills in all the required information for this type.

[Jongmassey]

@iaindillingham I've renamed the "metrics" method to something more appropriate. I hope the other changes are as expected, I'd appreciate a re-review :)

[iaindillingham]

This looks great, @Jongmassey. I've left a couple of comments, but if you just wanted to merge, then I'd completely understand 🙂. Thank you for all your hard work.

[iaindillingham]

I guess we can't pass table.primary_key instead of constraint here, because table.primary_key.name == None?

If that's the case, then consider naming the primary key when you define the table and removing the logic that determines the value of constraint from upsert. Something like:

GitHubCodespaces = Table(
    "github_codespaces",
    metadata,
    Column("created_at", TIMESTAMP(timezone=True)),
    Column("organisation", Text),
    Column("repo", Text),
    Column("user", Text),
    Column("last_used_at", TIMESTAMP(timezone=True)),
    PrimaryKeyConstraint(
        "created_at", "organisation", "repo", "user", name="github_codespaces_pkey"
    ),
)

[Jongmassey]

By just passing table.primary_key it actually passes a set of columns, from which it infers the correct constraint based on the columns. With compound primary keys, this is known to be a bit broken! The way I've written it, it handles either explicitly or defaultly-named PK constraints, either single or multiple columns. I feel this is a pretty robust strategy!

I did actually initially write this with an explicit PrimaryKeyConstraint in the table definition, but felt that defaulting to the default constraint name was a bit cleaner.