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.

Sign up for GitHub

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

Jump to bottom

custom listenaddr and endpoints for plugins #1654

Merged
merged 27 commits into from
Sep 7, 2022
Merged

custom listenaddr and endpoints for plugins #1654

merged 27 commits into from
Sep 7, 2022

Conversation

o0Ignition0o
Copy link
Contributor

@o0Ignition0o o0Ignition0o commented Aug 30, 2022 •
edited
Loading

❗ BREAKING ❗

The prometheus endpoint now listens to 0.0.0.0:9090/metrics by default. It previously listened to http://0.0.0.0:4000/plugins/apollo.telemetry/prometheus

Have a look at the Features section to learn how to customize the listen address and the path

🚀 Features

Allow users to customize the prometheus endpoint URL (#1645)

You can now customize the prometheus endpoint URL in your yml configuration:

telemetry:
  metrics:
    prometheus:
      listen: 0.0.0.0:9090 # default
      path: /metrics # default
      enabled: true

@github-actions

This comment has been minimized.

Sign in to view
@o0Ignition0o o0Ignition0o changed the title super early wip 🙈 wip: custom listenaddr and endpoints for plugins Aug 31, 2022
@o0Ignition0o o0Ignition0o changed the title wip: custom listenaddr and endpoints for plugins custom listenaddr and endpoints for plugins Aug 31, 2022
@o0Ignition0o o0Ignition0o marked this pull request as ready for review September 2, 2022 18:42
@o0Ignition0o
Copy link
Contributor Author

Still needs a changelog entry but I feel like its ready for (code) review

bnjjj
bnjjj reviewed Sep 5, 2022
View reviewed changes
Copy link
Contributor

@bnjjj bnjjj left a comment

Choose a reason for hiding this comment

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

You still have some TODOs. I don't know if you plan to address them in this PR or not. Also I'm not convinced about the naming web_endpoints but I don't have better name :p. Good job !

apollo-router/src/axum_http_server_factory.rs Show resolved Hide resolved
apollo-router/src/axum_http_server_factory.rs
// TODO [igni]: I believe configuring the router
// To listen to port 0 would lead it to change ports on every restart Oo

// serve main router

// if we received a TCP listener, reuse it, otherwise create a new one
#[cfg_attr(not(unix), allow(unused_mut))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's now useless. Not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we know who can answer this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any of us develop on Windows. You could just try removing it and then start a circelCI build and see how the Windows build proceeds...

apollo-router/src/axum_http_server_factory.rs
// populate the new listen addrs
for (listen_addr, routers) in extra_routers.into_iter() {
// if we received a TCP listener, reuse it, otherwise create a new one
#[cfg_attr(not(unix), allow(unused_mut))]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this.

apollo-router/src/router.rs Outdated Show resolved Hide resolved
garypen
garypen reviewed Sep 6, 2022
View reviewed changes
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Great work! A few nits, but nothing substantial.

apollo-router/src/axum_http_server_factory.rs Outdated Show resolved Hide resolved
apollo-router/src/axum_http_server_factory.rs

let router = make_axum_router(service_factory, &configuration, plugin_handlers)?;
// TODO [igni]: I believe configuring the router
// To listen to port 0 would lead it to change ports on every restart Oo
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's already an issue. I guess you are noting that it's an issue for --hot-reload since that might be surprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's not new, it was just a side by thought, I ll open an issue about that and we'll see if we wanna fix this

apollo-router/src/axum_http_server_factory.rs Outdated Show resolved Hide resolved
apollo-router/src/axum_http_server_factory.rs
Comment on lines +1423 to +1426
let url = format!(
"{}/graphql",
server.graphql_listen_address().as_ref().unwrap()
);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: We are doing this a lot. There must be a way to make this DRYer... I know this is an existing problem, but you see it and you can't un-see it..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha that's fair enough, let's open a followup

apollo-router/src/plugins/telemetry/metrics/prometheus.rs Outdated Show resolved Hide resolved
@o0Ignition0o o0Ignition0o enabled auto-merge (squash) September 7, 2022 07:59
@o0Ignition0o o0Ignition0o merged commit 839cf62 into main Sep 7, 2022
@o0Ignition0o o0Ignition0o deleted the igni/custom_endpoints branch September 7, 2022 08:19
@o0Ignition0o o0Ignition0o mentioned this pull request Sep 7, 2022
Closed
@o0Ignition0o o0Ignition0o linked an issue Sep 7, 2022 that may be closed by this pull request
Remove custom_endpoint and provide alternative functionality #1645
Closed
This was referenced Sep 7, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnjjj bnjjj bnjjj left review comments

@garypen garypen garypen left review comments

@BrynCooke BrynCooke BrynCooke approved these changes

@abernix abernix Awaiting requested review from abernix

@StephenBarlow StephenBarlow Awaiting requested review from StephenBarlow

Assignees

@o0Ignition0o o0Ignition0o

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove custom_endpoint and provide alternative functionality
4 participants
@o0Ignition0o @garypen @BrynCooke @bnjjj