Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

replace loggingcontexts with contextvars #10342

Open
richvdh opened this issue Jul 8, 2021 · 7 comments
Open

replace loggingcontexts with contextvars #10342

richvdh opened this issue Jul 8, 2021 · 7 comments
Labels
A-Logging Synapse's logs (structured or otherwise). Not metrics. P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Comments

@richvdh
Copy link
Member

richvdh commented Jul 8, 2021

logcontexts are super confusing, and easy to mess up.

We should experiment with replacing the thread-local variable we currently use with a contextvar, which Twisted has first-class support for (twisted/twisted#1192), which could potentially save everyone who ever touches Synapse code a great deal of pain.

@richvdh
Copy link
Member Author

richvdh commented Jul 8, 2021

It's worth noting that contextvars is new as of Python 3.7, but there is a backport.

It's also worth mentioning that twisted/twisted#1192 is new as of Twisted 21.2.0 (2021-02-28), so won't be in downstream distros for a while yet.

@richvdh richvdh changed the title replace logcontexts with contextvars replace loggingcontexts with contextvars Jul 8, 2021
@richvdh
Copy link
Member Author

richvdh commented Jul 8, 2021

Another fly in this ointment is that we rely on the fact that you have to manually switch into loggingcontexts to track CPU usage stats. (every time we switch loggingcontext, we record the CPU usage time). Currently I don't have any great ideas for solving that.

@anoadragon453 anoadragon453 added T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases labels Jul 9, 2021
@callahad
Copy link
Contributor

Python 3.6 goes EOL at the end of 2021; flagging this to re-review at the start of next year so we don't have the added complexity of a backport of contextvars

@callahad callahad added this to the Revisit: Yearly milestone Sep 15, 2021
@richvdh
Copy link
Member Author

richvdh commented Nov 29, 2021

Another fly in this ointment is that we rely on the fact that you have to manually switch into loggingcontexts to track CPU usage stats. (every time we switch loggingcontext, we record the CPU usage time). Currently I don't have any great ideas for solving that.

One idea: stick a shim high in the call stack which clocks a coroutine through manually, measuring CPU usage:

@types.coroutine
def measure_coro_rusage(coro, rusage) -> Generator:
    total_cpu_utime = 0
    val_to_send = None
    while True:
        start_cpu_utime = resource.getrusage().ru_utime
        try:
            try:
                result = coro.send(val_to_send)
            except StopIteration as e:
                return e.value
        finally:
            rusage.ru_utime += resource.getrusage().ru_utime - start_cpu_utime

        val_to_send = yield result

Probably needs more handling for edge cases, as well as sending exceptions back into the coroutine, but you get the general idea.

@callahad callahad removed this from the Revisit: Next Year milestone Jan 6, 2022
@callahad callahad added P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches and removed Z-Future-Maintenance Things that can't yet be done, but will need cleaning up in a couple of months/releases labels Jan 6, 2022
@richvdh
Copy link
Member Author

richvdh commented Jan 31, 2022

bad news: as of current Twisted, contextvars support still appears broken: https://twistedmatrix.com/trac/ticket/10301

That said, it may well be possible to work around this by avoiding use of reset and tracking the "previous value" ourselves.

@richvdh
Copy link
Member Author

richvdh commented Jan 31, 2022

One idea: stick a shim high in the call stack which clocks a coroutine through manually, measuring CPU usage:

incidentally, I think this might go some way to solving #8073: we currently do a lot of getrusage calls for entering/leaving nested logging contexts where we don't really care about the cpu usage stats.

@DMRobertson
Copy link
Contributor

contextvars support still appears broken: twistedmatrix.com/trac/ticket/10301

Fixed in Twisted 22.8.0 (xref #14207).

@MadLittleMods MadLittleMods added the A-Logging Synapse's logs (structured or otherwise). Not metrics. label Jul 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Logging Synapse's logs (structured or otherwise). Not metrics. P3 (OBSOLETE: use S- labels.) Approved backlog: not yet scheduled, will accept patches T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

No branches or pull requests

5 participants