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

Update router bridge and add option for reuse_query_fragments #3453

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

BrynCooke
Copy link
Contributor

@BrynCooke BrynCooke commented Jul 18, 2023

A new option has been added to the Router to allow disabling of the reuse of query fragments. This is useful for debugging purposes.

supergraph:
  experimental_reuse_query_fragments: false

The default value depends on the version of federation.

Fixes #3452

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

@github-actions

This comment has been minimized.

@BrynCooke BrynCooke closed this Jul 18, 2023
@BrynCooke BrynCooke reopened this Jul 18, 2023
@router-perf
Copy link

router-perf bot commented Jul 18, 2023

CI performance tests

  • xxlarge-request - Stress test with 100 MB request payload
  • step - Basic stress test that steps up the number of users over time
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • large-request - Stress test with a 1 MB request payload
  • const - Basic stress test that runs with a constant number of users
  • no-graphos - Basic stress test, no GraphOS.
  • reload - Reload test over a long period of time at a constant rate of users
  • xlarge-request - Stress test with 10 MB request payload
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity

@BrynCooke BrynCooke marked this pull request as ready for review July 18, 2023 11:21
@BrynCooke BrynCooke enabled auto-merge (squash) July 18, 2023 11:50
@BrynCooke BrynCooke merged commit c9d2b83 into dev Jul 18, 2023
2 checks passed
@BrynCooke BrynCooke deleted the federation-fragments branch July 18, 2023 13:24
@glasser
Copy link
Member

glasser commented Jul 18, 2023

For posterity — the description of this PR wasn't 100% right. The query fragment reuse feature (and the flag to disable it at the JS level) was introduced in Federation v2.1.0, not v2.4.9. What v2.4.9 introduced was a bug in that feature; its change was rolled back in v2.4.10.

The new router-bridge release contains both the 2.4.9->2.4.10 upgrade which rolls back the bug, and exposes the flag that disables the overall feature in Rust (with the default value being true as determined by the JS code).

My intention when exposing the flag was to keep the same default value (ie, router can pass reuse_query_fragments: None to router-bridge which means it would inherit the default value of true from JS), but that wasn't super clear in our communication.

So this PR here actually does more than remove the newly-added buggy aspect of the RQF feature — it actually completely disables it, which wasn't the intention. (The intention was to remove the new bug and let users turn the feature off, but not to change the feature's default enablement.)

I believe @BrynCooke is working on a PR to keep the default behavior the same now.

BrynCooke added a commit that referenced this pull request Jul 18, 2023
…ult on or off (#3462)

PR #3453 Incorrectly defaulted reuse query fragments to off. This is now
controlled by federation.

<!-- start metadata -->

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]. It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding
Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`

Co-authored-by: bryn <bryn@apollographql.com>
@BrynCooke BrynCooke mentioned this pull request Jul 19, 2023
BrynCooke added a commit that referenced this pull request Jul 20, 2023
> **Note**
>
> When approved, this PR will merge into **the `1.25.0` branch** which
will — upon being approved itself — merge into `main`.
>
> **Things to review in this PR**:
> - Changelog correctness (There is a preview below, but it is not
necessarily the most up to date. See the _Files Changed_ for the true
reality.)
>  - Version bumps
>  - That it targets the right release branch (`1.25.0` in this case!).
>
---
## 🚀 Features

### Persisted Queries w/opt-in safelisting (preview) ([PR
#3347](#3347))

> ⚠️ **Persisted queries is an [Enterprise
feature](https://www.apollographql.com/blog/platform/evaluating-apollo-router-understanding-free-and-open-vs-commercial-features/)
of the Apollo Router.** It requires an organization with a [GraphOS
Enterprise plan](https://www.apollographql.com/pricing/) and the feature
to be enabled for your account.
>
> If your organization _doesn't_ currently have an Enterprise plan, you
can test out this functionality by signing up for a free [Enterprise
trial](https://www.apollographql.com/docs/graphos/org/plans/#enterprise-trials)
and reaching out to enable the feature for your account.

Persisted Queries gives you the tools to prevent unwanted traffic from
reaching your graph.

It has two modes of operation:
* **Unregistered operation monitoring**
* Your router can allow all GraphQL operations, while emitting
structured traces containing unregistered operation bodies.
* **Operation safelisting**
  * Reject unregistered operations
  * Require all operations to be sent as an ID

Unlike automatic persisted queries (APQ), the ability to create a
safelist of operations allows you to prevent a malicious actor from
constructing a free-format query that could overload your subgraphh
services.

For more information con how to register queries and configure your
router see the [Persisted Query
documentation](https://www.apollographql.com/docs/graphos/routing/persisted-queries).

By [@EverlastingBugstopper](https://github.com/EverlastingBugstopper) in
#3347

## 🐛 Fixes

### Fix prometheus statistics issues with _total_total names([Issue
#3443](#3443))

When producing prometheus statistics the otel crate (0.19.0) now
automatically appends `_total` which is unhelpful.

This fix removes `_total_total` from our statistics. However, counter
metrics will still have `_total` appended to them if they did not so
already.

By [@garypen](https://github.com/garypen) in
#3471

### Enforce default buckets for metrics ([PR
#3432](#3432))

When `telemetry.metrics.common` was not configured, no default metrics
buckets were configured.
With this fix by default it set these buckets: `[0.001, 0.005, 0.015,
0.05, 0.1, 0.2, 0.3, 0.4, 0.5, 1.0, 5.0, 10.0]`

By [@bnjjj](https://github.com/bnjjj) in
#3432

## 📃 Configuration

### Add `subscription.enabled` field to enable subscription support
([Issue #3428](#3428))

`enabled` is now required in `subscription` configuration. Example:

```yaml
subscription:
  enabled: true
  mode:
    passthrough:
      all:
        path: /ws
```

By [@bnjjj](https://github.com/bnjjj) in
#3450

### Add option to disable reuse of query fragments ([Issue
#3452](#3452))

A new option has been added to the Router to allow disabling of the
reuse of query fragments. This is useful for debugging purposes.
```yaml
supergraph:
  experimental_reuse_query_fragments: false
```

The default value depends on the version of federation.

By [@BrynCooke](https://github.com/BrynCooke) in
#3453

## 🛠 Maintenance

### Coprocessor: Set a default pool idle timeout duration. ([PR
#3434](#3434))

The default idle pool timeout duration in Hyper can sometimes trigger
situations in which an HTTP request cannot complete (see [this
comment](hyperium/hyper#2136 (comment))
for more information).

This changeset sets a default timeout duration of 5 seconds.

By [@o0Ignition0o](https://github.com/o0Ignition0o) in
#3434

---------

Co-authored-by: bryn <bryn@apollographql.com>
Co-authored-by: Chandrika Srinivasan <chandrikas@users.noreply.github.com>
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.

Reuse of query fragments issue
3 participants