Simplified prom route when using default express route

[kennsippell]

#112

[kennsippell]

I made a new test server in this PR because the default route (wildcard '*') assumes a paradigm where requests get handled by routes which don't call next().

In the existing tests, routes always call next() and the test logic relies on this for the error cases.

Open to ideas on how to better integrate this with existing tests.

[kennsippell]

There are three cases we need to worry about the default route.

1. Routine route assignment (ln65)
2. Appending when includeQueryParams is true (ln76)
3. Nest.js case is actually setting route to :0* (ln82)

Because this affected so many branches, I opted for a short-circuit. Alternatives and suggestions welcome.

[kobik]

as it seems we're missing support * in general.
according to express documentation

This route path will match abcd, abxcd, abRANDOMcd, ab123cd, and so on.

app.get('/ab*cd', (req, res) => {
  res.send('ab*cd')
})

and also regex support, which we can ignore for now.

shouldn't we support all routes with * instead?
the main issue here is that we'd need to also support * with request params. i.e

app.get('/ab*cd/:id', (req, res) => {
  res.send('ab*cd')
})

this isn't a must for now, but would have been a more complete solution.

lmk what you think.

[kennsippell]

I'd be interested to pursue the general solution. If somebody has an answer to this question, I'll have a better understanding of the requirements and can take a stab at it. Without that, I'm worried I'm going to break something.

I'll install Nest and play around if I don't heard back.

[kennsippell]

Before merge, let me test this on some production data and confirm this is really what we want & need :) I'll send an update on this shortly.

[kennsippell]

Tested today and the data is much much cleaner. Also found #114.

[kobik]

sorry for the very late response.

is this still relevant?

[kennsippell]

@kobik This is still relevant for our project. We are running in production with issues patched. Would be great to somehow get these into an official release.

[kobik]

OK, let's drive this forward @kennsippell.

After going over the discussion we had on the change, I'd like to be able to support * in general if you don't mind.
As I answered to your question about why the request parameters are mutated, it is because otherwise we could have high cardinality due to the path parameters, i.e. calling /users/:id with thousands of users.

Ofc I agree that the code shouldn't mutate the original request params, yet we need to make sure that we don't use the request params values for our metrics.

[kennsippell]

Sorry for the year lull there. @kobik I've added three tests to confirm the expected behavior for the cases called out.

Here case app.get('/hello/:var'
Here case app.get('/ab*cd'
Here case app.get('/ab*cd'/:var

Does it seem to you like the * case is handled generically? Can you call out any other cases of concern you can think of?

[kennsippell]

I will mention that this case still fails (behavior same as in production today).

            before(() => {
                return supertest(app)
                    .get('/hellllllo/val')
                    .expect(200)
                    .then((res) => {});
            });
            it('should add it to the histogram', () => {
                return supertest(app)
                    .get('/metrics')
                    .expect(200)
                    .then((res) => {
                        expect(res.text).to.contain('method="GET",route="/he*o/:val",code="200"');
                    });
            });

If I use .get('/hellllllo/variable') it works. If I use .get('/hellllllo/val') it fails. This is that case when the user's input matches name of the route variable. This is because of the block discussed here which is apparently needed to support Nest.js.

I don't understand what scenario this code is for. I believe it to be bugged. There are no tests for it.