Progressive override support #4521
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
CI performance tests
|
trevor-scheer
force-pushed
the
trevor/progressive-override-plugin
branch
from
ed08b96
to
9be2786
Compare
This comment has been minimized.
This comment has been minimized.
trevor-scheer
force-pushed
the
trevor/progressive-override-plugin
branch
from
07ae1fe
to
0fe90a7
Compare
trevor-scheer
changed the title
Geal
reviewed
Jan 25, 2024
apollo-router/src/spec/schema.rs
Outdated
| }) | ||
| } | ||
|
|
||
| pub(crate) fn directive_name( | ||
| schema: &apollo_compiler::schema::Schema, | ||
| url: &str, | ||
| base_url: &str, | ||
| min_version: &str, |
There was a problem hiding this comment.
why only min_version? If we change a directive to add a field and expect a meaningful behaviour change from it, a router that does not know how to interpret that directive should reject it
There was a problem hiding this comment.
Let me know what you think here
9803b4b
There was a problem hiding this comment.
Is this implementation meant to align with the Versioning section of the Core specification? (I started an internal conversation about link vs core, but for the purposes of this PR, I am mostly just questioning whether — for review purposes — the intent is for this to match that implementation as defined in the core spec, irregardless of the overall spec-status itself.)
This reverts commit 5fd4a48.
trevor-scheer
force-pushed
the
trevor/progressive-override-plugin
branch
from
0d66273
to
af91538
Compare
Geal
approved these changes
Jan 30, 2024
| .build(), | ||
| SchemaRestriction::Directive { | ||
| name: "@authenticated".to_string(), | ||
| url: "https://specs.apollo.dev/authenticated/v0.1".to_string(), |
There was a problem hiding this comment.
a topic for a follow up PR: should the schema restrictions also work on a version range? Or a URL prefix like https://specs.apollo.dev/authenticated/?
There was a problem hiding this comment.
Thanks for pointing this out. I resolved a couple related bugs and implemented the version range in a separate PR.
#4570
* Require schema spec enforcements to specify a version req * Require directive argument enforcements to specify a version req * Handle `as` renames gracefully for the directive argument enforcement * Search the schema for actual usages instead of the definition for directive argument enforcements
This reverts commit e0cba72.
trevor-scheer
commented
Jan 31, 2024
The initial implementation of directive argument schema enforcement is lacking: * It doesn't handle renames of the `join` spec via `as`, meaning the restriction could be circumvented via a `join` rename. * It only looked for the directive argument on the definition rather than actual usages. If the arg is defined but unused, we shouldn't enforce anything. It might exist in the definition simply as a result of using the latest versions of subgraph libs/composition. Additionally, this PR introduces the requirement of specifying a `semver::VersionReq` for enforcement.
This was referenced Feb 1, 2024
Closed
Merged
Geal
added a commit
that referenced
this pull request
Feb 5, 2024
## 🚀 Features ### Specify Trace ID Formatting ([PR #4530](#4530)) This adds the ability to specify the format of the trace ID in the response headers of the supergraph service. An example configuration making use of this feature is shown below: ```yaml telemetry: apollo: client_name_header: name_header client_version_header: version_header exporters: tracing: experimental_response_trace_id: enabled: true header_name: trace_id format: decimal # Optional, defaults to hexadecimal ``` If the format is not specified, then the trace ID will continue to be in hexadecimal format. By [@nicholascioli](https://github.com/nicholascioli) in #4530 ### Introduce support for progressive `@override` ([PR #4521](#4521)) The change brings support for progressive `@override`, which allows dynamically overriding root fields and entity fields in the schema. This feature is enterprise only and requires a license key to be used. A new `label` argument is added to the `@override` directive in order to indicate the field is dynamically overridden. Labels can come in two forms: 1) String matching the form `percent(x)`: The router resolves these labels based on the `x` value. For example, `percent(50)` will route 50% of requests to the overridden field and 50% of requests to the original field. 2) Arbitrary string matching the regex `^[a-zA-Z][a-zA-Z0-9_-:./]*$`: These labels are expected to be resolved externally via coprocessor. A coprocessor a supergraph request hook can inspect and modify the context of a request in order to inform the router which labels to use during query planning. Please consult the docs for more information on how to use this feature and how to implement a coprocessor for label resolution. By [@trevorscheer](https://github.com/TrevorScheer) in #4521 ### Add a new selector to get all baggage key values in span attributes ([Issue #4425](#4425)) If you have several baggage items and would like to add all of them directly as span attributes, for example `baggage: my_item=test, my_second_item=bar` will add 2 span attributes `my_item=test` and `my_second_item=bar`. Example of configuration: ```yaml telemetry: instrumentation: spans: router: attributes: baggage: true ``` By [@bnjjj](https://github.com/bnjjj) in #4537 ### Create a trace during router creation and plugin initialization ([Issue #4472](#4472)) When the router starts or reload, it will now generate a trace with spans for query planner creation, schema parsing, plugin initialization and request pipeline creation. This will help in debugging any issue during startup, especially in plugins creation By [@Geal](https://github.com/Geal) in #4480 ### Add static attribute on specific span in telemetry settings ([Issue #4561](#4561)) Example of configuration: ```yaml telemetry: instrumentation: spans: router: attributes: "my_attribute": "constant_value" supergraph: attributes: "my_attribute": "constant_value" subgraph: attributes: "my_attribute": "constant_value" ``` By [@bnjjj](https://github.com/bnjjj) in #4566 ## 🐛 Fixes ### Order HPA target so that kubernetes does not rewrite ([Issue #4435](#4435)) This update addresses an OutOfSync issue in ArgoCD applications when Horizontal Pod Autoscaler (HPA) is configured with both memory and CPU limits. Previously, the live and desired manifests within Kubernetes were not consistently sorted, leading to persistent OutOfSync states in ArgoCD. This change implements a sorting mechanism for HPA targets within the Helm chart, ensuring alignment with Kubernetes' expected order. This fix proactively resolves the sync discrepancies while using HPA, circumventing the need to wait for Kubernetes' issue resolution (kubernetes/kubernetes#74099). By [@cyberhck](https://github.com/cyberhck) in #4436 ### Reactivate log events in traces ([PR #4486](#4486)) This fixes a regression introduced in #2999, where events were not sent with traces anymore due to too aggressive sampling By [@Geal](https://github.com/Geal) in #4486 ### Fix inconsistency in environment variable parsing for telemetry ([Issue #3203](https://github.com/apollographql/router/issues/ISSUE_NUMBER)) Previously, the router would complain when using the rover recommendation of `APOLLO_TELEMETRY_DISABLED=1` environment variable. Now any non-falsey value can be used, such as 1, yes, on, etc.. By [@nicholascioli](https://github.com/nicholascioli) in #4549 ### Store static pages in `Bytes` structure to avoid expensive allocation per request ([PR #4528](#4528)) The `CheckpointService` created by the `StaticPageLayer` caused a non-insignificant amount of memory to be allocated on every request. The service stack gets cloned on every request, and so does the rendered template. The template is now stored in a `Bytes` struct instead which is cheap to clone. By [@xuorig](https://github.com/xuorig) in #4528 ### Fix header propagation issues ([Issue #4312](#4312)), ([Issue #4398](#4398)) This fixes two header propagation issues: * if a client request header has already been added to a subgraph request due to another header propagation rule, then it is only added once * `Accept`, `Accept-Encoding` and `Content-Encoding` were not in the list of reserved headers that cannot be propagated. They are now in that list because those headers are set explicitely by the Router in its subgraph requests There is a potential regression: if a router deployment was accidentally relying on header propagation to compress subgraph requests, then it will not work anymore because `Content-Encoding` is not propagated anymore. Instead it should be set up from the `traffic_shaping` section of the Router configuration: ```yaml traffic_shaping: all: compression: gzip subgraphs: # Rules applied to requests from the router to individual subgraphs products: compression: identity ``` By [@Geal](https://github.com/Geal) in #4535 ## 🧪 Experimental ### Move cacheability metrics to the entity cache plugin ([Issue #4253](#4253)) The metric was generated in the telemetry plugin before, but it was not very convenient to keep it there. This adds more configuration: - enable or disable the metrics - set the metrics storage TTL (default is 60s) - the metric's typename attribute is disabled by default. Activating it can greatly increase the cardinality This also includes some cleanup and performance improvements By [@Geal](https://github.com/Geal) in #4469 --------- Co-authored-by: Edward Huang <edward.huang@apollographql.com> Co-authored-by: Jeremy Lempereur <jeremy.lempereur@iomentum.com> Co-authored-by: Jesse Rosenberger <git@jro.cc>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change brings support for progressive
@override, which allows dynamically overriding root fields and entity fields in the schema. This feature is enterprise only and requires a license key to be used.Support for this is implemented via plugin and support for a new context key whose value is provided as options to query planning.
We expect users to interact with and resolve labels themselves via coprocessors, so we're providing an interface via a known context key in order to communicate to and from. The coprocessors need to know which labels are available in the schema and they need to tell the router which labels to set.
We also expect users to use a "special-case" label format
percent(x)which the plugin will resolve out of the box.All of this is to say, the plugin has a few responsibilities:
router_service), the plugin will add all (non-percent) labels from the schema to the known context keyapollo_override::unresolved_labels.supergraph_request) may read from this context entry and append labels to be overridden to theapollo_override::labels_to_overridecontext entry.supergraph_request) will resolve percentage-based labels and append to theapollo_override::labels_to_overridecontext entry.apollo_override::labels_to_overridecontext entry to ensure the set of labels is minimized, sorted, etc.It's important to note that the set of overridden labels is an input to query planning, and thus becomes part of the cache key. For every relevant label in an operation, the number of potential QP cache entries doubles. Step 4 is important to ensure we don't create any more cache entries per operation than is necessary.
TODO:
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
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. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩