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

Reconsider the organization of configuration sections #1500

Closed
SimonSapin opened this issue Aug 12, 2022 · 6 comments · Fixed by #1718
Closed

Reconsider the organization of configuration sections #1500

SimonSapin opened this issue Aug 12, 2022 · 6 comments · Fixed by #1718
Assignees

Comments

@SimonSapin
Copy link
Contributor

Let's have a look before 1.0 at how we want to split configuration keys into sections

@Geal
Copy link
Contributor

Geal commented Aug 12, 2022

so, the point I was raising there is that the configuration schema is too influenced by the plugins structure, and not enough by what would make sense from the usage point of view.

As an example, let's say we had a configuration file using URL override, header manipulation and rate limiting:

server:
  listen: 0.0.0.0:4000

override_subgraph_url:
  accounts: http://localhost:8080

headers:
  subgraphs:
    accounts:
      - propagate:
        named: "Authorization"
    products:
      - insert:
          name: "InsertSubgraph"
          value: "Value"

traffic_shaping:
  router:
    rate_limit:.
      capacity: 10
      interval: 5s
    timeout: 50sec
  subgraphs:
    products:
      rate_limit:
      timeout: 50sec

Here we see router level and subgraph level configuration in 3 different sections, because each one has its own configuration. This is still a manageable example, but it is bound to become more complex as more subgraphs are added and more plugins are developed.

What I would like to see is something of that shape:

server:
  listen: 0.0.0.0:4000
  traffic_shaping:
    rate_limit:.
      capacity: 10
      interval: 5s
    timeout: 50sec

subgraphs:
  accounts:
    override_url: http://localhost:8080
    headers:
      - propagate:
        named: "Authorization"
  products:
    headers:
      - insert:
        name: "InsertSubgraph"
        value: "Value"
    traffic_shaping:
      rate_limit:
      timeout: 50sec

Router level configuration is clearly separated, and each subgraph has its own configuration. Note that this is still the same configuration shape though, because each subsection still has the plugin name. I would like at some point that configuration be completely decorrelated from plugin name (example: by flattening the traffic_shaping options directly under the router or subgraph section), but that would be too much work for now.

What I am proposing would amount to extracting the global and subgraph level sections of a plugin from the configuration, and assembling it in a coherent configuration object for that plugin, which is much more doable than a complete configuration overhaul

@garypen
Copy link
Contributor

garypen commented Aug 18, 2022

Discussed with @SimonSapin and concluded that this does seem like a more natural way to specify configuration for a router administrator. However, it's probably not something we should hold up 1.0 for.

@SimonSapin
Copy link
Contributor Author

I would like at some point that configuration be completely decorrelated from plugin name

For external plugins that seems tricky to me.

A more specific change could be to keep config sections named after plugin names, but in addition to the router-global ones have optional per-subgraph config with a second associated type in the Plugin trait


Some possible changes for 1.0 IMO:

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Aug 25, 2022

We’ve decided not to change configuration for 1.0 PR: #1608

@SimonSapin SimonSapin removed this from the v1.0.0-alpha.0 milestone Aug 25, 2022
@abernix abernix linked a pull request Aug 31, 2022 that will close this issue
@abernix abernix modified the milestones: v1.0.0-alpha.1, v1.0.0-alpha.2, v1.0.0-alpha.3 Aug 31, 2022
@abernix
Copy link
Member

abernix commented Sep 6, 2022

From @garypen in #1608 (comment)

cors:
  allow_credentials: true
 # renamed from landing_page
sandbox:
  listen: 127.0.0.1:4000
  path: /my_sandbox_path
  enabled: true <= NOT MANDATORY?
graphql:
  listen: 127.0.0.1:4000
  # renamed from endpoint
  path: /my_graphql_path
  introspection: false
  preview_defer_support: false
telemetry:
  metrics:
    prometheus:
      listen: 0.0.0.0:9090 # default
      path: /metrics # default
      enabled: true <= REQUIRED?
health-check:
      listen: 0.0.0.0:9090 # default
      path: /health # default

@abernix abernix modified the milestones: v1.0.0-alpha.3, v1.0.0-alpha.4 Sep 7, 2022
@o0Ignition0o
Copy link
Contributor

@abernix did we define a default path for sandbox?
The current configuration won't be reproducible once we make this change, we will have a conflict between the graphql and the sandbox path.

@o0Ignition0o o0Ignition0o self-assigned this Sep 7, 2022
@abernix abernix modified the milestones: v1.0.0-alpha.4, v1.0.0-rc.0 Sep 12, 2022
o0Ignition0o added a commit that referenced this issue Sep 13, 2022
…le (#1718)

Fixes #1500

### Configuration: Update metrics and healthcheck web endpoints, and make them configurable ([#1500](#1500))

The web endpoints exposed by the router listen to 127.0.0.1 by default, and the ports and paths for health check and prometheus have changed.

Here's the list of the endpoints exposed by the router:

- GraphQL: http://127.0.0.1:4000/ (unchanged)
- The GraphQL sandbox: http://127.0.0.1:4000/ (unchanged)
- Prometheus metrics: http://127.0.0.1:9090/metrics (used to be http://127.0.0.1:4000/plugins/apollo.telemetry/prometheus)
- Healthcheck: http://127.0.0.1:9494/health (used to be http://127.0.0.1:4000/.well-known/apollo/server-health)

While you could previously only customize the path for these endpoints, you can now customize the full IP address, PORT and PATH.

In order to enable this new feature, various `server` attributes such as `listen`, `graphql_path` and `landing_page` moved to more relevant sections.
Likewise, `introspection` and `preview_defer_support` have moved from the `server` section to the `supergraph` section:

This previous configuration: 
```yaml
server:
  listen: 127.0.0.1:4000
  graphql_path: /graphql
  health_check_path: /health
  introspection: false
  preview_defer_support: true
  landing_page: true
telemetry:
  metrics:
    prometheus:
      enabled: true
```

Now becomes:
```yaml
# landing_page configuration
sandbox: 
  listen: 127.0.0.1:4000
  path: /
  enabled: false # default
# graphql_path configuration
supergraph:
  listen: 127.0.0.1:4000
  path: /
  introspection: false
  preview_defer_support: true
# health_check_path configuration
health-check:
  listen: 127.0.0.1:9494
  path: /health
  enabled: true # default
# prometheus scraper configuration
telemetry:
  metrics:
    prometheus:
      listen: 127.0.0.1:9090
      path: /metrics
      enabled: true
```

By [@o0Ignition0o](https://github.com/o0Ignition0o) in #1718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants