search

PayU / prometheus-api-metrics

API and process monitoring with Prometheus for Node.js micro-service
Apache License 2.0
130 stars 44 forks source link

Check if properRoute is found before acting on it #73

Closed kdhira closed 2 years ago

kdhira commented 2 years ago

In the cases where the request route is not matched to any Koa route, bail out before trying to format the path string.

Issue: #59

kdhira commented 2 years ago

@kobik can you take a look at this?

kobik commented 2 years ago

Hi @kdhira , thanks for the PR!

I'll have a look tomorrow.

kdhira commented 2 years ago

Thanks @kobik

Let me know if there's any questions or anything you're unsure about. Like I mentioned before, the fix minimally circumvents the NPE which gets the route label working with just 'N/A' for my usecase.

Also I'm hoping that these changes can be released to NPM as a version of prometheus-api-metrics, probably something like 3.2.1 of the package. Is there anything extra I need to do (ie version bumping etc) or is that all handled by you?

kdhira commented 2 years ago

Hi @kobik,

I added a unit test for this scenario. Please let me know if anything is wrong. I also have a commit for updating dependencies from npm audit fix.

kdhira commented 2 years ago

Thanks @kobik,

I've seen your comment regarding the package version, and have reverted it. I had this as a separate commit so it was simple for me to just rebase it out.

Let me know if there's anything else I should change :)

kobik commented 2 years ago

@kdhira, can you also revert the package-lock.json changes?

I'd like to handle them in a separate PR.

kdhira commented 2 years ago

@kobik, I have dropped my commit that updated dependencies (and doing so reverted the package-lock.json changes I made). Hope this helps!

kobik commented 2 years ago

Sure, thanks again @kdhira !

kobik commented 2 years ago

@kdhira it seems like the new test didn't cover our case.

see https://coveralls.io/builds/47157280/source?filename=src%2Fkoa-middleware.js#L99

kdhira commented 2 years ago

Hmm I'll have to look at the test again, but I'm pretty confident that line gets run as the test I made has to resolve the route to use in the metric as 'N/A' somehow (through the || 'N/A' in _handleResponse) ...

kdhira commented 2 years ago

Does the coveralls tool run the unit-tests or the integration tests (or both) to detect coverage?

kdhira commented 2 years ago

Hey @kobik,

I think I have done what I need to ensure coverage. I added an integration test, and updated the test koa-server implementation so that the wildcard endpoint case could be correctly tested.

I also ran npm run integration-tests and after this addition it no longer shows line 99 as being an Uncovered Line.

Should be ready to run the Github workflows again

kobik commented 2 years ago

you're correct, for some reason it's only collected from integration tests. you can update the workflow to run nyc npm test instead of running unit and integration separately, which should collect the coverage of both of them.

but also what you did should be fine 👍

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 3.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: