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

[Batching] Build upon the recent configuration changes to enable subgraph filtering #4780

Merged
merged 49 commits into from
Mar 12, 2024

Conversation

garypen
Copy link
Contributor

@garypen garypen commented Mar 11, 2024

Now that we have Batching subgraph configuration we can use it to control filtering by batching enabled subgraphs.

also:

  • Add a migration from experimental_batching -> batching
  • Add subgraph filtering utility function to batching configuration
  • Add some subgraph filtering unit tests
  • merge dev

Note: @nicholascioli I merged in dev, so for reviewing purposes, just look at this commit.

Ref: Ref: #4661

abernix and others added 30 commits February 16, 2024 09:11
Cross compiling the router from ARM to x86 encounters issues with deno
snapshots, so for now we will reactivate x86 builds for macos on
CircleCI
RustSec advisory: https://rustsec.org/advisories/RUSTSEC-2024-0019

We are updating the mio dependency due to a vulnerability that was
reported when it is used with named pipes on Windows. As far as we know
this is not affecting the Router.
Update our linux images
from: `2004:2022.04.1`
to: `2004:2024.01.1`

Still `ubuntu 20.04` based, but with more recent patches and not subject
to brown outs.
Update the proto to the latest version. The change was only to internal
fields.
RustSec advisory: https://rustsec.org/advisories/RUSTSEC-2024-0019

We are updating the mio dependency due to a vulnerability that was
reported when it is used with named pipes on Windows. As far as we know
this is not affecting the Router.
Cross compiling the router from ARM to x86 encounters issues with deno
snapshots, so for now we will reactivate x86 builds for macos on
CircleCI
Client request body was fully loaded in memory before decompressing, we
are now decompressing it as it goes
Follow-up to the v1.40.2 being officially released, bringing version
bumps and changelog updates into the `dev` branch.
This reverts commit 11fad3c from #4625
on account of discomfort with the solution and not being able to
concretely tie it back an actual fix for the root cause. We have
re-opened #4612 to track the need to get back to this.
Fix #4665 (docs)

The token can be stored in different headers, but it can also be carried
by a cookie. This adds cookies as an alternative source of tokens, and
allows multiple alternative sources
Fix #4651

This adds the ability to set static headers on HTTP requests to download
a JWKS from an identity provider

Co-authored-by: Coenen Benjamin <benjamin.coenen@hotmail.com>
shorgi and others added 18 commits March 7, 2024 15:41
Co-authored-by: Maria Elisabeth Schreiber <maria.schreiber@apollographql.com>
Co-authored-by: Geoffroy Couprie <apollo@geoffroycouprie.com>
Some tests were missing to ensure that files nested within other GraphQL
types still resolved correctly, so this commit addresses that.

Fixes #4709 

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [x] 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.
Existing integration tests for the file uploads plugin did not test for
non-nullable uploads. As this is realistically the more common case,
this commit adds a test to ensure that non-nullable files work as
expected.

This is necessary since the router does some substitution of the
variables for the file upload case which might cause validation issues
if not carefully tested.

Fixes #4708
To enable correlation between DataDog tracing and logs dd.trace_id must
appear as a span attribute and also on the root of each json formatted
log message.

Currently, users can output dd.trace_id on logs but it is located within
the span attributes, and therefore will not show as correlated in the
datadog UI.

Old:
```
{"timestamp":"2024-03-07T14:42:44.408899252Z","level":"ERROR","message":"test","target":"apollo_router::plugins::telemetry","spans":[{"http.route":"/","http.request.method":"POST","url.path":"/","dd.trace_id":"3377607226581119358","client.name":"","client.version":"","name":"router"},{"name":"supergraph"},{"graphql.operation.type":"query","name":"execution"},{"apollo.subgraph.name":"products","name":"fetch"}],"resource":{"service.version":"1.40.1","process.executable.name":"router","service.name":"bryn-router"}}
```

New
```
{"timestamp":"2024-03-07T14:42:44.408899252Z","level":"ERROR","message":"test","target":"apollo_router::plugins::telemetry","dd.trace_id":"3377607226581119358","spans":[{"http.route":"/","http.request.method":"POST","url.path":"/","dd.trace_id":"3377607226581119358","client.name":"","client.version":"","name":"router"},{"name":"supergraph"},{"graphql.operation.type":"query","name":"execution"},{"apollo.subgraph.name":"products","name":"fetch"}],"resource":{"service.version":"1.40.1","process.executable.name":"router","service.name":"bryn-router"}}
```
This is how things look now:


![image](https://github.com/apollographql/router/assets/747836/0f249685-64fb-40c0-8090-4422fb0ae55a)


Fixes #4763


<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] 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.

---------

Co-authored-by: bryn <bryn@apollographql.com>
Co-authored-by: Edward Huang <edward.huang@apollographql.com>
Fixes a table issue introduced in
#4711

---------

Co-authored-by: Edward Huang <edward.huang@apollographql.com>
This commit ups the timeout used when verifying that the router instance
started by the integration test works as expected.

This is a temporary solution to fix some flakiness in the tests as a
better solution would not depend so heavily on non-deterministic timings
of the router.
Document Redis TLS configuration

Resolves https://apollographql.atlassian.net/browse/DOC-62

---------

Co-authored-by: Geoffroy Couprie <apollo@geoffroycouprie.com>
Co-authored-by: Chandrika Srinivasan <chandrikas@users.noreply.github.com>
Follow-up to the v1.41.0 being officially released, bringing version
bumps and changelog updates into the `dev` branch.
Previously, the integration test would spin up the router with its
listener always binding to port 4000, which might cause tests to fail if
a different process were to bind to the same port.

This commit makes the test router always bind to an available port and
corrects all calls to the router to use this new port.

Fixes #4720
> [!NOTE]
>
> v1.41.1 replaces a failed publish of v1.41.0. The version number had
to be moved from v1.41.0 to v1.41.1, but the release is otherwise the
same. Apologies for the confusion!

Only the version numbers have changed.
Follow-up to the v1.41.1 being officially released, bringing version
bumps and changelog updates into the `dev` branch.
Now that we have Batching subgraph configuration we can use it to
control filtering by batching enabled subgraphs.

also:
 - Add a migration from `experimental_batching` -> `batching`
 - Add subgraph filtering utility function to batching configuration
 - Add some subgraph filtering unit tests
@garypen garypen requested a review from a team as a code owner March 11, 2024 11:29
@garypen garypen self-assigned this Mar 11, 2024
@router-perf
Copy link

router-perf bot commented Mar 11, 2024

CI performance tests

  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • 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.
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • 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
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • xlarge-request - Stress test with 10 MB request payload
  • step - Basic stress test that steps up the number of users over time

@garypen garypen changed the title Build upon the recent configuration changes to enable subgraph filtering [Batching] Build upon the recent configuration changes to enable subgraph filtering Mar 11, 2024
Copy link
Contributor

@nicholascioli nicholascioli left a comment

Choose a reason for hiding this comment

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

Looks good to me!

There seems to be a weird formatting issue, but if the linting test didn't catch it then maybe it's OK? My guess is that the linter doesn't look at macros.

apollo-router/src/services/subgraph_service.rs Outdated Show resolved Hide resolved
@garypen garypen merged commit 9743e8e into garypen/2002-subgraph-batching Mar 12, 2024
3 of 6 checks passed
@garypen garypen deleted the garypen/use-subgraph-config branch March 12, 2024 08:19
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.

8 participants