Report QualifiedCapture correctly in prometheus metrics

[akshaymankar]

For prometheus metrics middleware to be able to replace
/users/4f7cbd8c-5fe3-4818-94c0-8ae68460ba13 with /users/:uid, it needs to know
the paths in servant that exist. This is generated statically using the class
RoutesToPaths. This class had an overlappable instance for everything, this
caused to not notice when we created the QualifiedCapture type. In order to
ensure that we instantiate this class correctly, this commit removes this
catch-all instance and instantiate the class for every type that needs it
explicitly.

https://wearezeta.atlassian.net/browse/FS-224

Checklist

• The PR Title explains the impact of the change.
• The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
• changelog.d contains the following bits of information:
  • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

[pcapriotti]

Personally, I'd prefer to keep the overlappable instances around, and maybe have some more tests that check that the captures are collected correctly. Also, I think it's better to test against the actual galley/brig API, instead of an artificial one. The way it currently is will fail if we say accidentally delete the instance for QualifiedCapture (unlikely), but not if we introduce some other capturing combinator without the corresponding instance (much more likely).

[akshaymankar]

Personally, I'd prefer to keep the overlappable instances around, and maybe have some more tests that check that the captures are collected correctly. Also, I think it's better to test against the actual galley/brig API, instead of an artificial one. The way it currently is will fail if we say accidentally delete the instance for QualifiedCapture (unlikely), but not if we introduce some other capturing combinator without the corresponding instance (much more likely).

I removed the overlappable instances exactly because I want compiler to tell us about the missing instance when we introduce a new capturing combinator. Even if we delete the instance for QualifiedCapture, the compilation would fail (not just of the test, but at the point where we try to use the middleware).
I wanted to originally test against the brig/galley API, but I couldn't figure out any way to do that without creating another golden test and just ask people to be careful when it breaks. That said, please let me know if you have any ideas.

[pcapriotti]

I removed the overlappable instances exactly because I want compiler to tell us about the missing instance when we introduce a new capturing combinator. Even if we delete the instance for QualifiedCapture, the compilation would fail (not just of the test, but at the point where we try to use the middleware).

I understand that, but my point was that perhaps we can achieve the same effect with a test, instead of having all these trivial instances around, which need to be extended every time we add a new combinator, even if it has no connection to metrics.

I wanted to originally test against the brig/galley API, but I couldn't figure out any way to do that without creating another golden test and just ask people to be careful when it breaks. That said, please let me know if you have any ideas.

Why? Couldn't we call getRoutes on say the brig api, and assert that the result contains a few of the things that we expect?

In any case, this is a minor point. If you disagree that having all these trivial instances is annoying, feel free to merge anyway.

[akshaymankar]

Why? Couldn't we call getRoutes on say the brig api, and assert that the result contains a few of the things that we expect?

How would this ensure that a new capture type has a correct instance? I guess it would tell us if we replace our current type, but if we use it in a new endpoint, we will still not know.

If you disagree that having all these trivial instances is annoying

I don't disagree, the trivial instances are annoying. But I don't know how to not write a good enough test which ensures that all the endpoints are correctly reported in metrics.

[pcapriotti]

How would this ensure that a new capture type has a correct instance? I guess it would tell us if we replace our current type, but if we use it in a new endpoint, we will still not know.

Such a test might have caught the problem solved by this PR though, because we created a new combinator and used in existing routes. But you're right, it won't catch cases where we create a new combinator and only use it in new routes.

I don't disagree, the trivial instances are annoying. But I don't know how to not write a good enough test which ensures that all the endpoints are correctly reported in metrics.

Feel free to merge, it's really not a big deal.