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

Run the guacgql HTTP server on only one port #1805

Conversation

nchelluri
Copy link
Contributor

@nchelluri nchelluri commented Apr 2, 2024

Description of the PR

Currently, if I check out main and run go run ./cmd/guacgql --gql-debug, then I will be able to access / (playground), /query (GQL server), and /metrics on both ports 8080 and 9091 of localhost. I don't think that this makes sense so I have changed the behavior in this PR so that all of these paths are served off of just one port.

To test this change, run each of the following commands in a terminal tab:

  1. docker run -p 4222:4222 -p 8222:8222 nats -m 8222 -js
  2. go run ./cmd/guacgql --gql-debug
  3. go run ./cmd/guaccsub
  4. go run ./cmd/guaccollect deps_dev

Then you will be able to access the GraphQL playground on http://localhost:8080, the GraphQL server metrics endpoint on http://localhost:8080/metrics, and the guaccollect deps.dev collector metrics on http://localhost:9091/metrics.

In addition, you may add a --gql-listen-port nnn to the second command and see the port for the GraphQL URLs change to nnn, and/or add a --prometheus-port mmm to the fourth command and see the port for the collector metrics change to mmm.

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If OpenAPI spec is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nchelluri nchelluri force-pushed the nchelluri/fix-gql-deps_dev-prometheus-port-conflict branch 2 times, most recently from 0e70208 to f4c007f Compare April 2, 2024 12:21
- Previously guacgql would run http.DefaultServeMux on
  two separate ports. One of them would conflict with
  the port used by Prometheus for guaccollect deps_dev.
  (The second port was used to provide Prometheus's
  /metrics route, but this was already present on the
  initial port, and it still is.)
- Fixes guacsec#1676

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri nchelluri force-pushed the nchelluri/fix-gql-deps_dev-prometheus-port-conflict branch from f4c007f to 452e79b Compare April 2, 2024 12:27
Copy link
Collaborator

@jeffmendoza jeffmendoza left a comment

Choose a reason for hiding this comment

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

Yes, makes more sense to have a /metrics path on the same port.

@nchelluri
Copy link
Contributor Author

Yes, makes more sense to have a /metrics path on the same port.

One note: it was actually on the same port before; http.DefaultServeMux was being served off of two separate ports (both of which had the / - the playground - and /metrics). I tried to clarify in the commit msg.

pkg/cli/init.go Show resolved Hide resolved
pkg/cli/init.go Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit ef4658e into guacsec:main Apr 3, 2024
8 checks passed
arorasoham9 pushed a commit to arorasoham9/guac that referenced this pull request Apr 4, 2024
- Previously guacgql would run http.DefaultServeMux on
  two separate ports. One of them would conflict with
  the port used by Prometheus for guaccollect deps_dev.
  (The second port was used to provide Prometheus's
  /metrics route, but this was already present on the
  initial port, and it still is.)
- Fixes guacsec#1676

Signed-off-by: Narsimham Chelluri (Narsa) <narsa@kusari.dev>
@nchelluri nchelluri deleted the nchelluri/fix-gql-deps_dev-prometheus-port-conflict branch April 8, 2024 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] HTTP address of metrics endpoints clash
3 participants