expose the custom endpoints from RouterServiceFactory #1402

Geal · 2022-07-15T09:43:02Z

the StateMachine was holding a copy of the plugin list to extract the
custom endpoints registered by some of the plugins, like the prometheus
endpoint.

With the Tower refactoring, that plugin list was empty so the endpoints
were not registered.

This commit makes sure that the plugins are only held byt the
RouterServiceFactory instance, and that it implements a method to return
the list of endpoints, that can be used by the StateMachine instance

the StateMachine was holding a copy of the plugin list to extract the custom endpoints registered by some of the plugins, like the prometheus endpoint. With the Tower refactoring, that plugin list was empty so the endpoints were not registered. This commit makes sure that the plugins are only held byt the RouterServiceFactory instance, and that it implements a method to return the list of endpoints, that can be used by the StateMachine instance

garypen

Looks good. A few small NITS (which may be ignored) to reduce use noise and perhaps improve readability.

apollo-router/src/state_machine.rs

Co-authored-by: Gary Pennington <[email protected]>

abernix

LGTM. One changelog consistency thing. I have a more meta question about why the test here wasn't failing, but that can be answered elsewhere (the answer is probably state machines):

router/apollo-router/src/plugins/telemetry/mod.rs

Lines 1278 to 1287 in d5a3a01

    
           let http_req_prom = http_ext::Request::fake_builder() 
        
               .uri(Uri::from_static( 
        
                   "http://localhost:4000/plugins/apollo.telemetry/prometheus", 
        
               )) 
        
               .method(Method::GET) 
        
               .body(Bytes::new()) 
        
               .build() 
        
               .unwrap(); 
        
           let resp = handler.oneshot(http_req_prom).await.unwrap(); 
        
           assert_eq!(resp.status(), StatusCode::OK);

NEXT_CHANGELOG.md

abernix · 2022-07-15T10:22:53Z

NEXT_CHANGELOG.md

+### Expose the custom endpoints from RouterServiceFactory [PR #1402](https://github.com/apollographql/router/pull/1402)
+
+Plugin HTTP endpoints registration was broken during the Tower refactoring. We now make sure that the list
+of endpoints is generated from the `RouterServiceFactory` instance.


❤️ (`<3`)

apollo-router/src/state_machine.rs

Co-authored-by: Jesse Rosenberger <[email protected]>

Geal · 2022-07-15T10:31:04Z

LGTM. One changelog consistency thing. I have a more meta question about why the test here wasn't failing, but that can be answered elsewhere (the answer is probably state machines):

it was testing the plugin HTTP handlers directly, but here the issue was in how the state machine codes was taking those handlers and setting them up in axum

@garypen

# [0.12.0] - 2022-08-18 ## ❗ BREAKING ❗ ### Move `experimental.rhai` out of `experimental` [PR #1365](#1365) You will need to update your YAML configuration file to use the correct name for `rhai` plugin. ```diff - plugins: - experimental.rhai: - filename: /path/to/myfile.rhai + rhai: + scripts: /path/to/directory/containing/all/my/rhai/scripts (./scripts by default) + main: <name of main script to execute> (main.rhai by default) ``` You can now modularise your rhai code. Rather than specifying a path to a filename containing your rhai code, the rhai plugin will now attempt to execute the script specified via `main`. If modules are imported, the rhai plugin will search for those modules in the `scripts` directory. for more details about how rhai makes use of modules, look at [the rhai documentation](https://rhai.rs/book/ref/modules/import.html). The simplest migration will be to set `scripts` to the directory containing your `myfile.rhai` and to rename your `myfile.rhai` to `main.rhai`. By [@garypen](https://github.com/garypen) in #1365 ## 🐛 Fixes ### The opentelemetry-otlp crate needs a http-client feature [PR #1392](#1392) The opentelemetry-otlp crate only checks at runtime if a HTTP client was added through cargo features. We now use reqwest for that. By [@Geal](https://github.com/geal) in #1392 ### Expose the custom endpoints from RouterServiceFactory ([PR #1402](#1402)) Plugin HTTP endpoints registration was broken during the Tower refactoring. We now make sure that the list of endpoints is generated from the `RouterServiceFactory` instance. By [@Geal](https://github.com/geal) in #1402 ## 🛠 Maintenance ### Dependency updates [PR #1389](#1389) [PR #1394](#1394) [PR #1395](#1395) Dependency updates were blocked for some time due to incompatibilities: - #1389: the router-bridge crate needed a new version of `deno_core` in its workspace that would not fix the version of `once_cell`. Now that it is done we can update `once_cell` in the router - #1395: `clap` at version 3.2 changed the way values are extracted from matched arguments, which resulted in panics. This is now fixed and we can update `clap` in the router and related crates - #1394: broader dependency updates now that everything is locked - #1410: revert tracing update that caused two telemetry tests to fail (the router binary is not affected) By [@Geal](https://github.com/Geal) in #1389 #1394 #1395 and [@o0Ignition0o](https://github.com/o0Ignition0o) in #1410

This comment has been minimized.

Sign in to view

fix test, changelog

08e23ef

Geal requested review from garypen and abernix July 15, 2022 09:51

garypen approved these changes Jul 15, 2022

View reviewed changes

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

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

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

Apply suggestions from code review

d329f62

Co-authored-by: Gary Pennington <[email protected]>

Geal enabled auto-merge (squash) July 15, 2022 10:19

abernix approved these changes Jul 15, 2022

View reviewed changes

abernix reviewed Jul 15, 2022

View reviewed changes

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

Geal disabled auto-merge July 15, 2022 10:28

Apply suggestions from code review

39750a4

Co-authored-by: Jesse Rosenberger <[email protected]>

lint

61dbdb2

Geal enabled auto-merge (squash) July 15, 2022 10:42

Geal merged commit d06c0c3 into main Jul 15, 2022

Geal deleted the geal/expose-endpoints-from-routerservicefactory branch July 15, 2022 11:00

o0Ignition0o added this to the v0.12.0 milestone Jul 18, 2022

o0Ignition0o mentioned this pull request Jul 18, 2022

release: v0.12.0 #1411

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expose the custom endpoints from RouterServiceFactory #1402

expose the custom endpoints from RouterServiceFactory #1402

Geal commented Jul 15, 2022

This comment has been minimized.

garypen left a comment

abernix left a comment •

edited

Loading

abernix Jul 15, 2022

Geal commented Jul 15, 2022

	let http_req_prom = http_ext::Request::fake_builder()
	.uri(Uri::from_static(
	"http://localhost:4000/plugins/apollo.telemetry/prometheus",
	))
	.method(Method::GET)
	.body(Bytes::new())
	.build()
	.unwrap();
	let resp = handler.oneshot(http_req_prom).await.unwrap();
	assert_eq!(resp.status(), StatusCode::OK);

expose the custom endpoints from RouterServiceFactory #1402

expose the custom endpoints from RouterServiceFactory #1402

Conversation

Geal commented Jul 15, 2022

This comment has been minimized.

garypen left a comment

Choose a reason for hiding this comment

abernix left a comment • edited Loading

Choose a reason for hiding this comment

abernix Jul 15, 2022

Choose a reason for hiding this comment

Geal commented Jul 15, 2022

abernix left a comment •

edited

Loading