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

Add metricsapp option #101

Merged
merged 6 commits into from
Jun 18, 2022

Conversation

ath88
Copy link
Contributor

@ath88 ath88 commented Jun 17, 2022

Fixes #25

Exposing Node.js internals on a public facing API is potentially a security issue. This lets you expose the metrics endpoint on a different node app listening on a different port.

Lets you supply an additional express app on which to mount the metrics 
endpoint
@disjunction
Copy link
Collaborator

Thanks for the contribution! Yet I find the suggested solution outside of the library better:
#25 (comment)

The pull request is lacking 2 items:

  • types/index.d.ts needs to be extended to include the new option
  • the code should be covered in unit tests

@ath88
Copy link
Contributor Author

ath88 commented Jun 17, 2022

Thanks for the contribution! Yet I find the suggested solution outside of the library better: #25 (comment)

The pull request is lacking 2 items:

  • types/index.d.ts needs to be extended to include the new option
  • the code should be covered in unit tests

I see your point. It's cleaner and doesn't pollute the code base and is fully supported by autoregister. But - I see a two benefits over the solution mentioned in the comments:

  • No need to exclude the metrics path from being instrumented. Prometheus will already record timing and and response codes.
  • Being opinionated about security. With collectDefaultMetrics you'll expose your Node.js version to the world, if you forget to block it in your firewall or proxy or whatever you have. This is an issue that people have. By supporting this directly, people won't have to look through closed issues to find the secure approach.

I added the requested changes. :)

@disjunction disjunction merged commit 1b52c6f into jochen-schweizer:master Jun 18, 2022
@disjunction
Copy link
Collaborator

@ath88 thanks again! This has been published to npm as version 6.5.0

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

Successfully merging this pull request may close these issues.

Question: How do I configure a different port for the metrics
2 participants