seanh
commented
2 days ago Re-based this and pushed a small change to use %r and {obj!r} when using GroupMembership's in logging and formatting, to make sure it uses __repr__() not __str__(). There is no GroupMembership.__str__() anyway so this makes no functional difference, but it's more correct.
Add logging to
GroupMembersServicewhenever creating or deleting a group membership. I think we should start doing this as a matter of good practice: each view or service method that creates, updates or deletes something in the DB should log the changes it makes. This could have helped with a recent bug (https://github.com/hypothesis/h/pull/9080) in a number of ways:Logging helps with writing unittests: tests can assert that expected messages were logged, which helps to ensure that the test is going down the code path that it expects to.
Logging can also help with manual testing either locally or on staging or production: you can look at the logs being produced and are more likely to notice if it's doing something wrong (e.g. deleting too many objects)
Logging can help with analysing the impact of a bug and with data recovery.
For example with the bug above it actually wasn't possible to figure for certain which group memberships had been erroneously deleted and restore them. The logging included in this commit would have made complete recovery easy: we actually could have restored the deleted memberships from the log messages alone, without even needing a DB snapshot.
For more complex objects (e.g. annotations, users, groups) it's not possible to include all attributes of the object in a single log line so you couldn't recover deleted objects from the logs alone. But if the logs include enough information to identify the deleted objects in a DB snapshot (e.g. if they included the primary key) they can help ensure that full recovery is both possible and quick and easy to do.