Remove metrics listener type in favor of http listener with metrics resource
#18584
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Remove `metrics` listener (serves metrics at `/metrics`) type in favor of `http` listener with `metrics` resource (serves metrics at `/_synapse/metrics`). | ||
|
There was a problem hiding this comment. It's unclear to me why There was a problem hiding this comment. One thing that @erikjohnston pointed out is that the dedicated Looking at the git history, the threading aspect just seems like the way it was originally implemented and not something explicitly called out for the benefit described above. There was a problem hiding this comment. Even with the updated conclusion from #18592 (comment) which most likely means we will continue to use the global (assuming we can't come up with a benefit/compelling reason for why we want both) There was a problem hiding this comment. Having thought about it a bit: I think it is worth keeping the separate Even if we keep the separate listener, I think we should still make them render from the same source and remove any differences. If needed its easy enough to change the There was a problem hiding this comment.
How sure are we that we care about this? Feels like a use case that we're just holding onto especially when we can't find any evidence. From what I'm reading,
I don't see an easy way forward as I don't know how to start another Twisted server in another thread in order to share the I can pull out some of the clean-up from this PR to ship separately. There was a problem hiding this comment. We relatively often have episodes on matrix.org where the reactor tick times are in the multiples of seconds or tens of seconds — this seems like a case where the separate metrics thread is a winner to me, so that metric exposition continues to work in this situation. The reason why this helps, despite the GIL, is because the metrics thread can preempt the reactor thread between bytecode boundaries and the metrics thread gets scheduled with roughly equal priority to the reactor thread, whereas (afaik) there isn't an easy mechanism to do something similar between separate twisted tasks/deferreds on the same reactor (and deferreds are co-operatively scheduled so we can't actually force one to yield to another). For illustration: if you want to disrupt metrics reporting for the reactor version, all you need to do is The more realistic case is likely just having lots of tasks in the reactor queue. In this case, the mental model is that the metrics request is scheduled with roughly equal priority to all the user requests (which, in some dodgy scenarios, there could be thousands of). There was a problem hiding this comment. Thanks for the extra details @reivilibre 🙂 That proves well enough that this actually works how we want and we've already expressed interest in wanting the benefit ⏩ There was a problem hiding this comment. Here is some prior art that I missed earlier that confirms why we care about having this: Lines 41 to 45 in fc10a5e And traced it back to the PR that introduced this language, matrix-org/synapse#3274, which also states:
When I originally traced back to where the There was a problem hiding this comment. In addition to the docs we already have, I've also added the context and @reivilibre's explanation for why it works to some comments in the code itself, see #18687 |
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinions on calling out the upgrade notes from the changelog entry itself? It looks like usually we prefer to call it out at the top of the changelog section for the version. Doesn't seem like it would hurt to have it in both places. The only problem being the chicken and egg problem of the upgrade notes being written later in order to have the link filled in.