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
docs: Document multiple app approach #4393
Conversation
## Running Multiple Instances | ||
<a id="multiple"></a> | ||
|
||
There are several use-cases where running multiple Fastify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me why this setup is better?
The metrics
data is calculated from the API server
itself.
If you are exposing healthcheck
and metrics
in a dedicated instance. Does it means that you are create an extra proxy?
Reverse Proxy ---> Metrics Server ---> API Server
Then, why don't you setup the metrics handler inside Reverse Proxy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least our situation is that while it is possible to use an elaborate nginx setup to only expose a subset of endpoints, our platform team is asking us to expose metrics on a separate port instead, as that would reduce complexity on their end, and this seems to be a fairly popular practice in the industry.
Would you recommend otherwise?
It would be something close to this: SkeLLLa/fastify-metrics#43 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, two port for a single application certainly increase the complexity (including both server and applications).
Imagine when you try to scale up horizontally, which means the port open is always double and shouldn't be the same.
Moreover, taking nginx as configuration example. It requires to use two different upstream instead of sharing one. It actually duplicating works.
It is completely fine if this is hard requirement for your company. But I wouldn't recommend to others.
I can give out a more solid example for why it only increase complexity in later time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a hard requirement, it's an open discussion between engineers and platform. If one is preferable over the other, we should clearly document it and then follow it.
@mcollina Can you chime in on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
our platform team is asking us to expose metrics on a separate port instead, as that would reduce complexity on their end, and this seems to be a fairly popular practice in the industry.
Is it? This is the first time I have heard of doing such a thing. In fact, it makes no sense to me. The healthcheck/metrics endpoint, on a different port, is going to give you insight into an instance that is not processing requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing a separate port is the recommended approach by prometheus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exposing a separate port is the recommended approach by prometheus.
I search a bit around and couldn't found any related article about this approach. And it is not on prometheus
website documentation either.
May I know where you read this information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know where else this fits, but it does not seem like a recommendation to me. We are not actually recommending the user to implement multiple instance, but merely stating that it is possible and acceptable.
@jsumners We should decide which approach we recommend and document it. If reverse proxy for healthchecks and metrics is recommended instead, it should go there. See quarkusio/quarkus#7893 for a discussion on this. It is definitely quite a popular pattern. |
Co-authored-by: James Sumners <james@sumners.email>
I don't recall that being a recommendation in the reverse proxy rec. The reverse proxy needs to access the healthcheck endpoint on the backend services in order to determine if the instance is available to route to. And that is another point in favor of "I don't know why anyone would do this"; the healthcheck needs to indicate that the process serving requests is good. |
@jsumners What about smart healthchecks, which potentially ping external services and do db heartbeat? Exposing them as a public endpoint might be a ddos risk. |
I'm not entirely convinced JVM requirements translate well to other runtimes. But the linked discussion is clearly talking about the same instance serving both ports: app (8080) and metrics (9090). This PR is talking about (effectively) two separate processes. I do think this is better solved at the proxy, or ingress firewall, by blocking public access to the metrics endpoints. |
Throughout the issue you posted. The contributor also support to not spiting the port for If the concern is security, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
My only objection is that we don't have a good place for this "call out". I suppose if it were its own guide that details the utility of helthcheck endpoints, this could be a section in that document. |
…ultiple-instances � Conflicts: � docs/Guides/Recommendations.md
…cs/multiple-instances
Can you please add a reference to prometheus recommendation of listening to another port? Take a look at https://github.com/platformatic/platformatic/blob/main/packages/db/lib/metrics-plugin.js to see how I would implement this. |
@mcollina I can't find it; there are plenty of examples all over the web, using |
I couldn't find it either.
The document stated the default behavior and it shouldn't be a recommendation.
The closest should be Writing exporters and it's recommendation conflict with the current wide spread usage of
It clearly stated that recommend to use port other than the one listed on
Am I read it wrongly? The link inside stated it should be an |
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
All prometheus documentation assumes that metrics are exposed on a separate port.
The anti-patterns refer to having a global server for the exporter. This is needed because Platformatic supports hot reloading and closing and restarting that server will conflict with the rest of the workflow. Thanks for catching, this required a bit longer an explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All prometheus documentation assumes that metrics are exposed on a separate port.
Just take the tutorial
as an example. It expose /metrics
the same as /ping
and have no word about separating port.
If there are any assumption, I don't know why it didn't mention a word about it.
From what I see, the document does mention you should secure your /metrics
endpoint to only accessible by prometheus
.
The method is not clearly stated and user can consider Basic Authentication
, Separate Port
like you mention. But, it shouldn't be a recommendation.
My only objection is that we don't have a good place for this "call out". I suppose if it were its own guide that details the utility of helthcheck endpoints, this could be a section in that document.
I agree with James, Recommendation
is not a good place for these information. It is leak of solid evidence.
It deserves it's own document page and more complete guide through on this area.
I've asked the masses https://twitter.com/matteocollina/status/1587810168766611460 |
...and we have an answer from one of the authors of Prometheus: https://twitter.com/juliusvolz/status/1587832390860382210?s=20&t=zUjhxsKzgt23VoIZWH8UQg. |
Saying what has already been said: "eh, whatever". |
It is more like common practices or company consideration, but not recommended approach. |
Sure, I'll make this less prescriptive. |
@climba03003 @jsumners What about now? I hope it's fairly clear that we are not encouraging people to use this approach, but are merely saying that it's OK to do so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Non-blocking, but it sounds strange to me a non-recommandation or explanation included inside a Recommandation
guides.
I'm not blocking this, but it really feels out of place. I still think it belongs in a new guide that provides the following:
|
Co-authored-by: James Sumners <james@sumners.email>
@jsumners But metrics aren't a focus of this section at all, it doesn't even recommend any particular way of doing them. The entire focus of this is |
Then it should be a callout in the Reviewing this document in whole again, I realize I should have fought harder for the Kubernetes section to be more thorough. In fact, it should probably be its own guide as well that details "how to run Fastify under Kubernetes". As it is written, it also isn't a recommendation, per se, and is another "this is how you do it" callout. |
Considering that there are two approves, and no blocks, I will very hesitantly merge this, as I believe that it is a net positive, even if not an ideal solution. From visibility perspective, I don't think that people looking up |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.io/) ([source](https://togithub.com/fastify/fastify)) | [`4.9.2` -> `4.10.2`](https://renovatebot.com/diffs/npm/fastify/4.9.2/4.10.2) | [![age](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/compatibility-slim/4.9.2)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/fastify/4.10.2/confidence-slim/4.9.2)](https://docs.renovatebot.com/merge-confidence/) | ### GitHub Vulnerability Alerts #### [CVE-2022-41919](https://togithub.com/fastify/fastify/security/advisories/GHSA-3fjj-p79j-c9hh) ### Impact The attacker can use the incorrect `Content-Type` to bypass the `Pre-Flight` checking of `fetch`. `fetch()` requests with Content-Type’s [essence](https://mimesniff.spec.whatwg.org/#mime-type-essence) as "application/x-www-form-urlencoded", "multipart/form-data", or "text/plain", could potentially be used to invoke routes that only accepts `application/json` content type, thus bypassing any [CORS protection](https://fetch.spec.whatwg.org/#simple-header), and therefore they could lead to a Cross-Site Request Forgery attack. ### Patches For `4.x` users, please update to at least `4.10.2` For `3.x` users, please update to at least `3.29.4` ### Workarounds Implement Cross-Site Request Forgery protection using [`@fastify/csrf`](https://www.npmjs.com/package/@​fastify/csrf). ### References Check out the HackerOne report: https://hackerone.com/reports/1763832. ### For more information [Fastify security policy](https://togithub.com/fastify/fastify/security/policy) --- ### Release Notes <details> <summary>fastify/fastify</summary> ### [`v4.10.2`](https://togithub.com/fastify/fastify/releases/tag/v4.10.2) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.10.1...v4.10.2) ####⚠️ Security Release⚠️ - Fix for ["Incorrect Content-Type parsing can lead to CSRF attack"](https://togithub.com/fastify/fastify/security/advisories/GHSA-3fjj-p79j-c9hh) and CVE-2022-41919 **Full Changelog**: fastify/fastify@v4.10.1...v4.10.2 ### [`v4.10.1`](https://togithub.com/fastify/fastify/releases/tag/v4.10.1) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.10.0...v4.10.1) #### What's Changed - fix node 19.1.0 port validation test by [@​Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4427 - Add fastify-constraints to community plugins by [@​Ceres6](https://togithub.com/Ceres6) in [fastify/fastify#4428 - build(deps-dev): bump [@​sinonjs/fake-timers](https://togithub.com/sinonjs/fake-timers) from 9.1.2 to 10.0.0 by [@​dependabot](https://togithub.com/dependabot) in [fastify/fastify#4421 - add silent option to LogLevel by [@​Uzlopak](https://togithub.com/Uzlopak) in [fastify/fastify#4432 #### New Contributors - [@​Ceres6](https://togithub.com/Ceres6) made their first contribution in [fastify/fastify#4428 **Full Changelog**: fastify/fastify@v4.10.0...v4.10.1 ### [`v4.10.0`](https://togithub.com/fastify/fastify/releases/tag/v4.10.0) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.9.2...v4.10.0) #### What's Changed - docs(reference/reply): spelling fixes by [@​Fdawgs](https://togithub.com/Fdawgs) in [fastify/fastify#4358 - Support different content-type typed reply with TypeProvider by [@​rain714](https://togithub.com/rain714) in [fastify/fastify#4360 - chore: remove leading empty lines by [@​LinusU](https://togithub.com/LinusU) in [fastify/fastify#4364 - fix types after pino 8.7.0 change by [@​mcollina](https://togithub.com/mcollina) in [fastify/fastify#4365 - Node.js V19 support by [@​mcollina](https://togithub.com/mcollina) in [fastify/fastify#4366 - fix: no check on `null` or `undefined` values passed as fn by [@​metcoder95](https://togithub.com/metcoder95) in [fastify/fastify#4367 - docs(server): config is lost when reply.call not found() is called by [@​cesarvspr](https://togithub.com/cesarvspr) in [fastify/fastify#4368 - Fix typo - 'sever' to 'server' by [@​utsav91](https://togithub.com/utsav91) in [fastify/fastify#4372 - Add platformatic to the Acknowledgements by [@​mcollina](https://togithub.com/mcollina) in [fastify/fastify#4378 - docs: add Simone Busoli to plugin maintainers by [@​simoneb](https://togithub.com/simoneb) in [fastify/fastify#4379 - add missing 'validationContext' field to FastifyError type by [@​jakubburzynski](https://togithub.com/jakubburzynski) in [fastify/fastify#4363 - fix(type-providers): assignability of instance with enabled type provider by [@​driimus](https://togithub.com/driimus) in [fastify/fastify#4371 - feat: support async trailer by [@​climba03003](https://togithub.com/climba03003) in [fastify/fastify#4380 - fix: trailers async race condition by [@​climba03003](https://togithub.com/climba03003) in [fastify/fastify#4383 - docs(ecosystem): Add fastify-list-routes by [@​chuongtrh](https://togithub.com/chuongtrh) in [fastify/fastify#4385 - build(deps-dev): bump [@​sinclair/typebox](https://togithub.com/sinclair/typebox) from 0.24.51 to 0.25.2 by [@​dependabot](https://togithub.com/dependabot) in [fastify/fastify#4388 - \[ Fix ] Improve error message for hooks check by [@​debadutta98](https://togithub.com/debadutta98) in [fastify/fastify#4387 - fix: tiny-lru usage by [@​climba03003](https://togithub.com/climba03003) in [fastify/fastify#4391 - Removes old note about named imports in ESM by [@​fox1t](https://togithub.com/fox1t) in [fastify/fastify#4392 - docs: Add section about capacity planning by [@​kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4386 - docs(recommendations): grammar fixes by [@​Fdawgs](https://togithub.com/Fdawgs) in [fastify/fastify#4396 - chore(doc): duplicated menu item by [@​Eomm](https://togithub.com/Eomm) in [fastify/fastify#4398 - feat: add request.routeOptions object by [@​debadutta98](https://togithub.com/debadutta98) in [fastify/fastify#4397 - docs: Document multiple app approach by [@​kibertoad](https://togithub.com/kibertoad) in [fastify/fastify#4393 - fix example using db decorator on fastify instance by [@​mmarti](https://togithub.com/mmarti) in [fastify/fastify#4406 - docs: fix removeAdditional refer by [@​shunyue1320](https://togithub.com/shunyue1320) in [fastify/fastify#4410 #### New Contributors - [@​rain714](https://togithub.com/rain714) made their first contribution in [fastify/fastify#4360 - [@​LinusU](https://togithub.com/LinusU) made their first contribution in [fastify/fastify#4364 - [@​cesarvspr](https://togithub.com/cesarvspr) made their first contribution in [fastify/fastify#4368 - [@​utsav91](https://togithub.com/utsav91) made their first contribution in [fastify/fastify#4372 - [@​jakubburzynski](https://togithub.com/jakubburzynski) made their first contribution in [fastify/fastify#4363 - [@​driimus](https://togithub.com/driimus) made their first contribution in [fastify/fastify#4371 - [@​chuongtrh](https://togithub.com/chuongtrh) made their first contribution in [fastify/fastify#4385 - [@​debadutta98](https://togithub.com/debadutta98) made their first contribution in [fastify/fastify#4387 - [@​mmarti](https://togithub.com/mmarti) made their first contribution in [fastify/fastify#4406 - [@​shunyue1320](https://togithub.com/shunyue1320) made their first contribution in [fastify/fastify#4410 **Full Changelog**: fastify/fastify@v4.9.2...v4.10.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/quirrel-dev/quirrel). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNC4yIiwidXBkYXRlZEluVmVyIjoiMzUuMTMxLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
fixes fastify/help#784
Checklist
npm run test
andnpm run benchmark
and the Code of conduct