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

docs: Document multiple app approach #4393

Merged
merged 10 commits into from Nov 7, 2022
Merged

Conversation

kibertoad
Copy link
Member

fixes fastify/help#784

Checklist

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Nov 1, 2022
@kibertoad kibertoad changed the title Document multiple app approach docs: Document multiple app approach Nov 1, 2022
## Running Multiple Instances
<a id="multiple"></a>

There are several use-cases where running multiple Fastify
Copy link
Member

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?

Copy link
Member Author

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)

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

@jsumners jsumners left a 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.

@kibertoad
Copy link
Member Author

@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>
@jsumners
Copy link
Member

jsumners commented Nov 1, 2022

If reverse proxy for healthchecks and metrics is recommended instead, it should go there.

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.

@kibertoad
Copy link
Member Author

@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.

@jsumners
Copy link
Member

jsumners commented Nov 1, 2022

See quarkusio/quarkus#7893 for a discussion on this. It is definitely quite a popular pattern.

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.

@climba03003
Copy link
Member

Throughout the issue you posted. The contributor also support to not spiting the port for metrics and health check.
The application usually recommended to sit behind a reverse proxy which is good for horizontal scalability, cache, load balancing, etc.

If the concern is security, the reverse proxy already solve the problem.
If the concern is complexity, using same port for a single application with reverse proxy definitely less complex.
If the concern is platform, I don't see why blocking the metrics to public is a problem to them? If they don't want to do the job, you can definitely check if the request comes from trusted proxy and block the return.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kibertoad
Copy link
Member Author

@jsumners @mcollina How do we proceed when there are split opinions on this? Should I proceed with merging or not?

@jsumners
Copy link
Member

jsumners commented Nov 1, 2022

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.

@mcollina
Copy link
Member

mcollina commented Nov 1, 2022

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.

@kibertoad
Copy link
Member Author

@mcollina I can't find it; there are plenty of examples all over the web, using :9090, but I don't see any references to where this is coming from. Closest to that would be https://prometheus.io/docs/introduction/first_steps/ scraping from 9090, but I'm not sure if this is a particularly strong reference, default port for Prometheus server is 9090.
Have you ever seen anything stronger?

@climba03003
Copy link
Member

climba03003 commented Nov 2, 2022

I can't find it

I couldn't find it either.

I'm not sure if this is a particularly strong reference, default port for Prometheus server is 9090.

The document stated the default behavior and it shouldn't be a recommendation.

Have you ever seen anything stronger?

The closest should be Writing exporters and it's recommendation conflict with the current wide spread usage of :9090.

For exporters for internal applications we recommend using ports outside of the range of default port allocations.

It clearly stated that recommend to use port other than the one listed on Wiki and :9090 is one of them that should be prevented.
The reason is actually simple, make your life easier and shouldn't be a strong reference.

https://github.com/platformatic/platformatic/blob/main/packages/db/lib/metrics-plugin.js

Am I read it wrongly? The link inside stated it should be an anti-pattern?
https://github.com/platformatic/platformatic/blob/69d4ffe2dc9945ca98d53206271d71155a4d28d8/packages/db/lib/metrics-plugin.js#L11-L15

docs/Guides/Recommendations.md Outdated Show resolved Hide resolved
docs/Guides/Recommendations.md Outdated Show resolved Hide resolved
kibertoad and others added 2 commits November 2, 2022 11:13
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
@mcollina
Copy link
Member

mcollina commented Nov 2, 2022

It clearly stated that recommend to use port other than the one listed on Wiki and :9090 is one of them that should be prevented.
The reason is actually simple, make your life easier and shouldn't be a strong reference.

All prometheus documentation assumes that metrics are exposed on a separate port.

Am I read it wrongly? The link inside stated it should be an anti-pattern?
https://github.com/platformatic/platformatic/blob/69d4ffe2dc9945ca98d53206271d71155a4d28d8/packages/db/lib/metrics-plugin.js#L11-L15

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.

Copy link
Member

@climba03003 climba03003 left a 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.

@mcollina
Copy link
Member

mcollina commented Nov 2, 2022

I've asked the masses https://twitter.com/matteocollina/status/1587810168766611460

@mcollina
Copy link
Member

mcollina commented Nov 2, 2022

...and we have an answer from one of the authors of Prometheus: https://twitter.com/juliusvolz/status/1587832390860382210?s=20&t=zUjhxsKzgt23VoIZWH8UQg.

@jsumners
Copy link
Member

jsumners commented Nov 2, 2022

Saying what has already been said: "eh, whatever".

@climba03003
Copy link
Member

...and we have an answer from one of the authors of Prometheus: https://twitter.com/juliusvolz/status/1587832390860382210?s=20&t=zUjhxsKzgt23VoIZWH8UQg.

It is more like common practices or company consideration, but not recommended approach.
I would like this information placed in a separated guides. For example, Metrics, Application Structure Consideration, etc.

@kibertoad
Copy link
Member Author

Sure, I'll make this less prescriptive.

@kibertoad
Copy link
Member Author

@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.
It's not about application structure per se, nor about metrics, useful part of this section is about performance implications of running multiple instances. Considering that we are covering several anti-patterns in other sections, I would say that confirming that something is not an anti-pattern loosely fits the overall recommendation topic. We don't have a more appropriate document right now, and I wouldn't start a new one just for this fairly small bit.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@climba03003 climba03003 dismissed their stale review November 4, 2022 06:07

Non-blocking, but it sounds strange to me a non-recommandation or explanation included inside a Recommandation guides.

@jsumners
Copy link
Member

jsumners commented Nov 4, 2022

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:

  1. Title(?): Collecting Metrics
  2. Introduction to what metrics are
  3. A list of ways to collect metrics (Prom, OTEL, etc.)
  4. A short example of adding one or more metrics plugins to a simple server
  5. This callout as an aside

Co-authored-by: James Sumners <james@sumners.email>
@kibertoad
Copy link
Member Author

@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 It's perfectly fine to run multiple applications on the same server, which is an important piece of information.

@jsumners
Copy link
Member

jsumners commented Nov 4, 2022

The entire focus of this is It's perfectly fine to run multiple applications on the same server, which is an important piece of information.

Then it should be a callout in the server.listen docs? You have said yourself this isn't a recommendation for anything. The intention of the doc being edited is to provide literal team approved recommendations for running Fastify based application in production.

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.

@kibertoad
Copy link
Member Author

kibertoad commented Nov 7, 2022

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 server.listen are going to be same people who would benefit from the knowledge described here, this is not about metrics per se, and Recommendations is already a fairly general-purpose collection of runtime gotcha wisdom. I agree that it would be great to make it more focused, but I don't think that can or should happen within this PR - and I would be very happy to move this section elsewhere after a better place for it appears.

@kibertoad kibertoad merged commit 675b00d into main Nov 7, 2022
@kibertoad kibertoad deleted the docs/multiple-instances branch November 7, 2022 11:38
Skn0tt pushed a commit to quirrel-dev/quirrel that referenced this pull request Jun 20, 2023
[![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/@&#8203;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
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4427
- Add fastify-constraints to community plugins by
[@&#8203;Ceres6](https://togithub.com/Ceres6) in
[fastify/fastify#4428
- build(deps-dev): bump
[@&#8203;sinonjs/fake-timers](https://togithub.com/sinonjs/fake-timers)
from 9.1.2 to 10.0.0 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4421
- add silent option to LogLevel by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4432

#### New Contributors

- [@&#8203;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
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[fastify/fastify#4358
- Support different content-type typed reply with TypeProvider by
[@&#8203;rain714](https://togithub.com/rain714) in
[fastify/fastify#4360
- chore: remove leading empty lines by
[@&#8203;LinusU](https://togithub.com/LinusU) in
[fastify/fastify#4364
- fix types after pino 8.7.0 change by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4365
- Node.js V19 support by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4366
- fix: no check on `null` or `undefined` values passed as fn by
[@&#8203;metcoder95](https://togithub.com/metcoder95) in
[fastify/fastify#4367
- docs(server): config is lost when reply.call not found() is called by
[@&#8203;cesarvspr](https://togithub.com/cesarvspr) in
[fastify/fastify#4368
- Fix typo - 'sever' to 'server' by
[@&#8203;utsav91](https://togithub.com/utsav91) in
[fastify/fastify#4372
- Add platformatic to the Acknowledgements by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4378
- docs: add Simone Busoli to plugin maintainers by
[@&#8203;simoneb](https://togithub.com/simoneb) in
[fastify/fastify#4379
- add missing 'validationContext' field to FastifyError type by
[@&#8203;jakubburzynski](https://togithub.com/jakubburzynski) in
[fastify/fastify#4363
- fix(type-providers): assignability of instance with enabled type
provider by [@&#8203;driimus](https://togithub.com/driimus) in
[fastify/fastify#4371
- feat: support async trailer by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4380
- fix: trailers async race condition by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4383
- docs(ecosystem): Add fastify-list-routes by
[@&#8203;chuongtrh](https://togithub.com/chuongtrh) in
[fastify/fastify#4385
- build(deps-dev): bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.24.51 to 0.25.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4388
- \[ Fix ] Improve error message for hooks check by
[@&#8203;debadutta98](https://togithub.com/debadutta98) in
[fastify/fastify#4387
- fix: tiny-lru usage by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4391
- Removes old note about named imports in ESM by
[@&#8203;fox1t](https://togithub.com/fox1t) in
[fastify/fastify#4392
- docs: Add section about capacity planning by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4386
- docs(recommendations): grammar fixes by
[@&#8203;Fdawgs](https://togithub.com/Fdawgs) in
[fastify/fastify#4396
- chore(doc): duplicated menu item by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4398
- feat: add request.routeOptions object by
[@&#8203;debadutta98](https://togithub.com/debadutta98) in
[fastify/fastify#4397
- docs: Document multiple app approach by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4393
- fix example using db decorator on fastify instance by
[@&#8203;mmarti](https://togithub.com/mmarti) in
[fastify/fastify#4406
- docs: fix removeAdditional refer by
[@&#8203;shunyue1320](https://togithub.com/shunyue1320) in
[fastify/fastify#4410

#### New Contributors

- [@&#8203;rain714](https://togithub.com/rain714) made their first
contribution in
[fastify/fastify#4360
- [@&#8203;LinusU](https://togithub.com/LinusU) made their first
contribution in
[fastify/fastify#4364
- [@&#8203;cesarvspr](https://togithub.com/cesarvspr) made their first
contribution in
[fastify/fastify#4368
- [@&#8203;utsav91](https://togithub.com/utsav91) made their first
contribution in
[fastify/fastify#4372
- [@&#8203;jakubburzynski](https://togithub.com/jakubburzynski) made
their first contribution in
[fastify/fastify#4363
- [@&#8203;driimus](https://togithub.com/driimus) made their first
contribution in
[fastify/fastify#4371
- [@&#8203;chuongtrh](https://togithub.com/chuongtrh) made their first
contribution in
[fastify/fastify#4385
- [@&#8203;debadutta98](https://togithub.com/debadutta98) made their
first contribution in
[fastify/fastify#4387
- [@&#8203;mmarti](https://togithub.com/mmarti) made their first
contribution in
[fastify/fastify#4406
- [@&#8203;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>
Copy link

github-actions bot commented Nov 8, 2023

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance overhead of a second fastify instance
5 participants