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 Prometheus resource usage metrics for middleware #7215

Closed
wants to merge 1 commit into from

Conversation

sethidden
Copy link
Contributor

@sethidden sethidden commented Jul 9, 2024

Sending a GET to /metrics will now output Prometheus metrics implemented by https://github.com/siimon/prom-client/
https://prometheus.io/docs/instrumenting/writing_clientlibs/#standard-and-runtime-collectors

  • Expose metrics on separate port
  • Add env var to disable prometheus

@sethidden sethidden requested a review from a team as a code owner July 9, 2024 14:50
Copy link

changeset-bot bot commented Jul 9, 2024

🦋 Changeset detected

Latest commit: 30aa02e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@vue-storefront/middleware Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sethidden sethidden changed the title Add resource usage metrics for middleware Add Prometheus resource usage metrics for middleware Jul 9, 2024
@sethidden sethidden marked this pull request as draft July 11, 2024 12:50
@@ -82,6 +83,8 @@ async function createServer<
res.end("ok");
});

app.get("/metrics", createPrometheusMetricsHandler());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not acceptable to expose this on the same port as the rest of the regular services. This should be a separate express app listening on another port.

@@ -82,6 +83,8 @@ async function createServer<
res.end("ok");
});

app.get("/metrics", createPrometheusMetricsHandler());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can be determined based on NODE_ENV

Comment on lines +11 to +13
* Express API handler that should be registered on the /metrics route
* so that Prometheus can periodically call the middleware's /metrics endpoint
* and scrape the metrics
Copy link
Collaborator

Choose a reason for hiding this comment

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

What kind of metrics? This endpoint will be open (no token or whatever), anyone can hit it. Is it safe to expose such data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#7215 (comment)

this will be a separate express instance hosted on another port. in our cloud only port 4000 is exposed, but if metrics will be in port e.g. 9090 it won't be passed through to the internet, and we only be available locally

@sethidden
Copy link
Contributor Author

Stale

@sethidden sethidden closed this Oct 3, 2024
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.

2 participants