Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logging to GroupMembersService #9100

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Nov 19, 2024

Add logging to GroupMembersService whenever 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 (#9080) in
a number of ways:

  1. 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.

  2. 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)

  3. 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.

@seanh seanh requested a review from marcospri November 19, 2024 15:26
Copy link
Member

@marcospri marcospri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nothing to add

Base automatically changed from refactor-member-leave to main November 21, 2024 10:52
@seanh seanh force-pushed the add-logging-to-GroupMembersService branch 2 times, most recently from 88255b2 to 02378d3 Compare November 21, 2024 10:53
Add logging to `GroupMembersService` whenever 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 (#9080) in
a number of ways:

1. 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.

2. 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)

3. 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.
@seanh seanh force-pushed the add-logging-to-GroupMembersService branch from 02378d3 to 6dba0a5 Compare November 21, 2024 10:54
@seanh
Copy link
Contributor Author

seanh commented Nov 21, 2024

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.

@seanh seanh merged commit 61b6952 into main Nov 21, 2024
9 checks passed
@seanh seanh deleted the add-logging-to-GroupMembersService branch November 21, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants