Be able to correlate timeouts in reverse-proxy layer in front of Synapse (pull request ID from header)

[MadLittleMods]

Be able to correlate timeouts in reverse-proxy layer in front of Synapse (pull request ID from header).

For matrix.org as an example, we can use this to pull the request ID from the cf-ray header that Cloudflare provides. Then use any cf-ray we would see in a 524 Gateway timeout response to look at the trace in Jaeger.

As suggested by @clokep, #13786 (comment).

Fix #13685

Alternative to #13786

Before	After

---

New config:

  listeners:
    - port: 8008
      tls: false
      type: http
      x_forwarded: true
+     request_id_header: "cf-ray"
      bind_addresses: ['::1', '127.0.0.1', '0.0.0.0']

---

Discussions around the new GET-74a2179568cf26ff-MSP vs the sequential request ID's GET-17:

• Be able to correlate timeouts in reverse-proxy layer in front of Synapse (pull request ID from header) #13801 (comment)
• Be able to correlate timeouts in reverse-proxy layer in front of Synapse (tag specific request headers) #13786 (comment)

---

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.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[MadLittleMods]

Because we don't use the simple sequential ID when this config is defined, it might harder to follow each request in the logs (have to remember a random string like GET-74a2179568cf26ff-MSP vs an incrementing integer, GET-17). But when the sequential number is sufficiently big, it's hard to remember as well.

In tests, we still use the same sequential request ID's since this config won't be defined there. We're not changing any default either so it's basically opt-in but the plan is to define this on matrix.org so we can correlate CloudFlare timeouts with the trace in Synapse.

Is there any strong preference for keeping the GET-17, POST-541 , etc? Any real world usefulness from debugging a big Synapse instance?

[turt2live]

it's often easier in my experience to know the ordering of requests from their number rather than timestamp, given punctuation can obscure the problem: 2022-09-13 12:15:08 and 2022-09-13 12:16:09 look very similar in a pile of logs, but they're actually a full minute apart. The request ID would be able to show that there was a thousand requests in the meantime, for example.

Not that it's a super strong argument though - would just be sad to see it go.

[clokep]

Personally I usually just copy a one of those and grep the logs to find matching log lines, I'm not sure there's much gained by them being incrementing.

As you mention this is also "opt-in", but I find it much simpler to have a single request ID you can use across all logs related to a request (e.g. this would let you pivot across logs + jaeger easier than #13801).

[squahtx]

Summarizing the discussion in #synapse-devs:

• reivilibre points out that the Cloudflare ray IDs are longer and more unwieldy than the current IDs. They'll take a bit more memory, make log lines longer and they may be harder to juggle as a human reading the logs.
• babolivier notes that Cloudflare ray IDs are more unique than the current incrementing IDs. Right now, if you grep through logs for a low request number, you'll get unrelated requests if the homeserver has been restarting a lot or if you're grepping through logs for multiple workers.
• There wasn't any objection from the Synapse team to losing the incrementing property of the current IDs (apart from turt2live above).

Overall, no strong objections.

[clokep]

Looks good overall, some comments on the docs though.

[MadLittleMods]

Thanks for the review and pushing forward discussion @clokep! Also thanks to everyone for chiming in to make sure this change is acceptable to move forward with 🐰