Strip number suffix from instance name to consolidate services that traces are spread over

[MadLittleMods]

Strip number suffix from instance name to consolidate services that traces are spread over.

The problem with many services is that it makes it hard to find which service has the trace you want, see jaegertracing/jaeger-ui#985

Previously, we split traces out into services based on their instance name like matrix.org client_reader-1, etc but there are many worker instances of the same client_reader so there is a lot to click through.

With this PR, all of the traces are just collected under the worker type like client_reader, event_persister 😇

Note: A Synapse worker instance name is an opaque string with the number convention only being our own thing for the matrix.org deployment. But seems pretty sensible to group things this way.

Before	After

Dev notes

import re
STRIP_INSTANCE_NUMBER_SUFFIX_REGEX = re.compile(r"[_-]?\d+$")
re.sub(STRIP_INSTANCE_NUMBER_SUFFIX_REGEX, "", "matrix.org client_reader-8")
# Result: "matrix.org client_reader"

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]

Did we get any benefit from splitting up the traces by instance or instance type before?

[erikjohnston]

The only reason I've been holding off from doing this is that we lose the "colouring" of the traces, i.e. it becomes much harder to tell when processing moves to a different process. Not sure if there's another way of achieving the same effect?

[reivilibre]

I'm afraid I would second Erik's sort-of objection here; although having to find the right service is a minor pain, I really find the colouring useful because it helps to spot the points where the processing moves over replication.

Perhaps something else to consider: I wonder if e.g. Grafana's interface for viewing traces is better for finding traces regardless of service?

[erikjohnston]

We could also do something like change the service name to the worker type? I.e. the instance name without the index. That way it'd be easier to find stuff (as you don't have to figure out the instance), and for the most part instances don't talk to the same type?

[clokep]

We could also do something like change the service name to the worker type?

This was going to be my suggestion!

We could also add a tag which is the entire instance name if you did want to filter by that. I think the only time that is useful is if you want to see what happens when an instance is under load or something, but...I don't think this is the right interface for that.

[MadLittleMods]

The only reason I've been holding off from doing this is that we lose the "colouring" of the traces, i.e. it becomes much harder to tell when processing moves to a different process.

I really find the colouring useful because it helps to spot the points where the processing moves over replication.

Ahhh, makes sense. I have been mostly looking at the RoomMessageListRestServlet which is all turqoise in one process.

For reference, an example that spans multiple processes is RoomStateEventRestServlet

just note that 'the instance name without the index' is not a generic Synapse concept; it's our matrix.org Ansible stuff that has this convention. To Synapse, the instance name is opaque and just so happens to include a number.,,

Any tips for how to accomplish because this is what I also saw looking at this yesterday and just went with - Synapse?

We could strip number suffixes from the opaque worker name. Can we consider that a safe-enough convention?

[reivilibre]

I wouldn't be too offended if we took an educated guess and stripped "[_-]?[0-9]+$" off the end of the worker name. I think it's mainly just us/developers that would be using Jaeger anyway, so I don't consider it too harmful.

[erikjohnston]

We could make it configurable?

[MadLittleMods]

I've updated to the sensible default of stripping off the number suffix. Our matrix.org worker naming convention seems sane and we can avoid the config.

[MadLittleMods]

We could also add a tag which is the entire instance name if you did want to filter by that. I think the only time that is useful is if you want to see what happens when an instance is under load or something, but...I don't think this is the right interface for that.

BTW +1 to including the real instance name as a tag. I've found it useful to filter out a specific process that was struggling before now. It also gives you a place to look for the right logs when viewing a trace.

Split that out to #13761

[MadLittleMods]

Thanks for the review @erikjohnston, @clokep, and @reivilibre 🕊