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

Metric collection is not always accurate #7

Closed
edclement opened this issue Dec 21, 2021 · 0 comments · Fixed by #8
Closed

Metric collection is not always accurate #7

edclement opened this issue Dec 21, 2021 · 0 comments · Fixed by #8
Assignees
Labels
bug Something isn't working p1

Comments

@edclement
Copy link
Contributor

I started doing real world testing against a Verdaccio instance running in a Docker container that uses this plugin and discovered that the metrics being collected were not 100% accurate. The counts of package downloads were always slightly less then what I was expecting based on the tests I was running.

I ran and debugged a local instance and performed the same tests that were producing inaccurate results. What I discovered is that the Express response finish event does not always fire. The plugin currently listens for the finish event in order to collect final response data and label metrics appropriately. You can see the line of code for this here.

There isn't a lot of documentation available on when the different Node/Express response event types fire. After doing some additional research, I found two good references:

Specifically from the 2nd link above:

When does the request-response cycle end? The first and the obvious answer is when the response is sent. Node.js has a default event that is fired when the response has been sent. This hook is res.on('finish').

99% of software developers use only “finish” event, but I should notice that not every request comes to the “finish” state, in the real life client can close the connection before the response has been sent. In this case Node.js emits only the res.on('close') event.

This is exactly the type of behavior I was seeing in my testing and debugging. It seems that sometimes the npm client kills the connection prior to Express thinking the full response has been sent. The solution seems to be to rely on the close event instead of the finish event on the response object.

@edclement edclement added the bug Something isn't working label Dec 21, 2021
@edclement edclement self-assigned this Dec 21, 2021
@Splaktar Splaktar added the p1 label Dec 21, 2021
edclement pushed a commit that referenced this issue Dec 22, 2021
…e and npm publishing

Fixes an issue where package download counters were not always incrementing properly (#7). Add an
option to collect default prometheus metrics (#4). Remove Babel from the build process and rely only
on TypeScript (#2). Rename configuration property `enabled` to `metricsEnabled` to avoid collision
with other plugin configurations that are merged into the configuration object passed to the plugin.
Enforce commit message formats using commitizen and commitlint. Generate CHANGELOG.md using
`standard-version.

BREAKING CHANGE: configuration option `enabled` changed to `metricsEnabled`
edclement pushed a commit that referenced this issue Dec 23, 2021
…y accurate

During real world testing, the count of package downloads almost always came in lower then it should
have. It seems that sometimes the npm client kills the connection prior to Express thinking the full
response has been sent. The solution seems to be to rely on the `close` event instead of the
`finish` event on the response object.

fix #7
edclement pushed a commit that referenced this issue Dec 23, 2021
…y accurate

During real world testing, the count of package downloads almost always came in lower then it should
have. It seems that sometimes the npm client kills the connection prior to Express thinking the full
response has been sent. The solution seems to be to rely on the `close` event instead of the
`finish` event on the response object.

fix #7
edclement pushed a commit that referenced this issue Dec 23, 2021
…y accurate

During real world testing, the count of package downloads almost always came in lower then it should
have. It seems that sometimes the npm client kills the connection prior to Express thinking the full
response has been sent. The solution seems to be to rely on the `close` event instead of the
`finish` event on the response object.

fix #7
@edclement edclement mentioned this issue Dec 23, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants