Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified prom route when using default express route #113

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Simplified prom route when using default express route #113

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
03d01de
Simplified prom route when using default express route
kennsippell Jun 28, 2023
86f18b3
Simpler test server
kennsippell Jun 28, 2023
8497363
Test case that default route is not always hit
kennsippell Jun 28, 2023
7723d8c
Merge branch 'master' into 112-default-route
kobik Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/express-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ class ExpressMiddleware {
route = route ? route + req.route.path : req.route.path;
}

// #112 - aggregated express default route
Copy link
Author

@kennsippell kennsippell Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@kennsippell kennsippell Jul 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (route === '*') {
return route;
}

if (!route || route === '' || typeof route !== 'string') {
route = req.originalUrl.split('?')[0];
} else {
Expand Down
74 changes: 74 additions & 0 deletions test/integration-tests/express/middleware-default-route-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
'use strict';
const { expect } = require('chai');
const Prometheus = require('prom-client');
const supertest = require('supertest');

let app, config;

describe('when using express framework (default route)', function() {
Copy link
Author

@kennsippell kennsippell Jun 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.timeout(4000);
before(() => {
config = require('./server/config');
config.useUniqueHistogramName = true;
app = require('./server/express-server-default-route');
});
after(() => {
Prometheus.register.clear();
delete require.cache[require.resolve('./server/express-server-default-route.js')];
delete require.cache[require.resolve('../../../src/index.js')];
delete require.cache[require.resolve('../../../src/metrics-middleware.js')];
});

describe('when start up with default route', () => {
describe('when calling default endpoint', () => {
before(() => {
return supertest(app)
.get('/dne')
.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="*",code="200"');
});
});
});

describe('when calling existing GET endpoint', () => {
before(() => {
return supertest(app)
.get('/hello')
.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="/hello",code="200"');
});
});
});

describe('when calling a GET endpoint with one query param', () => {
before(() => {
return supertest(app)
.get('/hello?test=test')
.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="*",code="200"');
});
});
});
});
});
18 changes: 18 additions & 0 deletions test/integration-tests/express/server/express-server-default-route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
'use strict';

const express = require('express');
const middleware = require('../../../../src/index.js').expressMiddleware;

const app = express();
app.use(middleware({ includeQueryParams: true }));

app.get('/hello', (req, res) => {
res.json({ message: 'Hello World!' });
})

app.all('*', (req, res) => {
res.status(200);
res.json({ message: 'Default' });
});

module.exports = app;