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.

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

Socket level events are not detected #932

Closed
slavab89 opened this issue Jun 14, 2022 · 2 comments
Closed

Socket level events are not detected #932

slavab89 opened this issue Jun 14, 2022 · 2 comments

Comments

@slavab89
Copy link

Describe the bug
We've noticed that our log entries describe cases where we receive a 400 response (Or 404 as the default in Koa) but they are not logged in the metrics.
This seems to happen when there are errors in the form of ECONNRESET - when the underlying socket of the request throws some error.

To Reproduce
Using express, we can imitate this behavior using the following code

const express = require('express')
const app = express();
const { createMiddleware, getSummary, getContentType } = require('@promster/express');

const port = 3000;

app.use('/metrics', async (req, res) => {
  req.statusCode = 200;

  res.setHeader('Content-Type', getContentType());
  res.end(await getSummary());
});

app.use(createMiddleware({ app }));

app.get('/', (req, res) => {
  res.send('Hello World!')
});

app.get('/error', (req, res) => {
  req.socket.emit('error', new Error('boom'));
});

app.listen(port);

Expected behavior
The /metrics route to record the request to the /error route.

Additional context
It seems that the response finished event does not trigger in such cases (code)
Using a module such as https://www.npmjs.com/package/on-finished does trigger the callback and record the request correctly

@tdeekens
Copy link
Owner

tdeekens commented Sep 7, 2022

Hi, thanks for the issue. I lost track of responding. I think it makes sense tracking such errors but it's also on the boundary of what logging is for to me. Cause in terms of labelling the response they're not the regular 404. Or at least I'd like to now if the 404 was sent or a ECONNRESET.

Maybe it should be another metric which contains these by error and status code. Do you have any preference here?

@tdeekens
Copy link
Owner

tdeekens commented Sep 7, 2022

Note, it would be nice to start with a test case proving that it doesn't work. Then we could make the test pass. Would you be able to contribute on this issue yourself?

@tdeekens tdeekens closed this as not planned Won't fix, can't repro, duplicate, stale Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants