Add logging to ObservableDeferred callbacks

[ShadowJonathan]

This PR adds some rudimentary logging to ObservableDeferred when callbacks throw an exception.

This also removes the TODO comments, my rationale for this is that ObservableDeferred is only meant to 'observe', and so callback errors (which were previously already just thrown away) only need to be logged, nothing else should be done with them.

Found while working on #9522

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <[email protected]>

[richvdh]

lgtm I think but I'd like another pair of eyes on it

[erikjohnston]

I think this makes sense. I do wonder if we want to log them as exceptions though, so things like sentry pick them up and we can fix it?

[ShadowJonathan]

I do wonder if we want to log them as exceptions though, so things like sentry pick them up and we can fix it?

@erikjohnston That sounds like a better idea (for also other places in the codebase), but i have no idea how i can do that, do i need to change warning to exception or error? Where can i see more details about how sentry hooks into synapse, anyway?

[erikjohnston]

I do wonder if we want to log them as exceptions though, so things like sentry pick them up and we can fix it?

@erikjohnston That sounds like a better idea (for also other places in the codebase), but i have no idea how i can do that, do i need to change warning to exception or error? Where can i see more details about how sentry hooks into synapse, anyway?

It happens ✨ automagically ✨ when you log an error or exception :)

[erikjohnston]

(It works by the sentry package adding a hook to the python logging library so that it can see all the logs that go past)

[ShadowJonathan]

👍 Then i'll change the warning to an error, I think that even without a sentry library, it'd make more sense anyways.

[clokep]

Re-requested review from @erikjohnston to take a look at the additional changes.

[ShadowJonathan]

Oh, yeah, i forgot to mention i changed it to exception, because internally it still calls to error, it just semantically looked more appropriate for me.

stdlib for logging, anyhow;

    def exception(self, msg, *args, exc_info=True, **kwargs):
        """
        Convenience method for logging an ERROR with exception information.
        """
        self.error(msg, *args, exc_info=exc_info, **kwargs)