Make json schema small #5061
Merged
Make json schema small #5061
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
added 6 commits
April 30, 2024 10:55
Had to work around the issue where the json schema library that we use doesn't like the uri-refs that Schemars generates.
… code in the router. Update the snapshot, down from 96k to 6k.
…dditional properties can't really be used with subschemas. Therefore when using extendable we duplicate the property definitions. This is not as bad as it sounds and still gets us a schema at 7.2k lines
This comment has been minimized.
This comment has been minimized.
|
CI performance tests
|
added 3 commits
May 2, 2024 13:04
We can't use the json schema to redact the config anymore because annotations are not surfaced across schema refs: Stranger6667/jsonschema-rs#403
BrynCooke
force-pushed
the
bryn/json-schema
branch
from
8bba4f4
to
59394fb
Compare
added 2 commits
May 2, 2024 14:58
…an't use the same one that we had before. Use the schema to extract the property names, then take the paths from the config, redacting anything that isn't in the schema.
BrynCooke
force-pushed
the
bryn/json-schema
branch
from
d7cf113
to
3023281
Compare
Geal
approved these changes
May 3, 2024
| configuration.headers.all.request.insert.name.<redacted>: 1 | ||
| configuration.headers.all.request.insert.value.<redacted>: 1 | ||
| configuration.headers.all.request.propagate.len: 2 |
There was a problem hiding this comment.
why are we losing propagate and remove?
There was a problem hiding this comment.
I've improved it a bit: e096eb1.
The reason things are different is that we can't use the schema metadata anymore to detect the type of the element in the config. I've done a reasonable approximation.
Co-authored-by: Geoffroy Couprie <apollo@geoffroycouprie.com>
Without filtering the query hashes during batch creation we could end up in a situation where we have additional query hashes in the batch. This manifests itself by batch queries failing with query hashes appearing as committed without ever having been registered in a batch. Filtering during batch creation is now matching the filtering at subgraph coordination.
…runs (#5023) As tends to be the case, our CI testing times have been driven up over time. While we currently have broad-stroke measurements on the CI pipeline, we don't yet have per-test insights. This makes it difficult to commit time to specific efforts, or measure their improvement. Further, the ergonomics over using the failed tests interface on CircleCI have been hindered by the lack of test reporting through well-defined testing output artifacts like Junit, popular with most other languages. Rust core hasn't yet promoted its structured `cargo test` output into a stable build which — if we wanted to measure tests by producing anything remotely coercible into JUnit — would mean we'd have to run `+nightly` runs of the tests. Using the `nightly` build of `cargo test` to get almost (but still not) JUnit seems unnecessarily risky. Given that this feature hasn't promoted from `+nightly` in what must be plural years and the allure of other more feature-complete testing tools like [Nextest](https://nexte.st/) — I decided to give Nextest a quick try. The quick test is seemingly successful and validating: It did not show any degradation in test execution time, is already showing promising nice wins in CircleCI interface experience, seems like an improvement to the CLI experience (image below, but also by [highlighting specific test failures in the UI](https://app.circleci.com/pipelines/github/apollographql/router/20805/workflows/09ba269e-b917-4d2e-ad52-68e80e14ab94/jobs/140871/tests)), unlocks other capabilities (like automatic retries on a per-test basis), and didn't require passing custom thread flags ([perhaps due to a better model for test execution](https://nexte.st/book/how-it-works)?).  ### Local vs CI / How to enable nextest locally Nextest is only used if it is installed and the decision to do so is made inside of `cargo xtask test` based on the presence of `cargo-nextest`. If `cargo-nextest` is **not** installed, then the existing `cargo test` is used as a fall-back. On CI, it is installed and thus used.
Updates our notion of `current` to the latest version `windows-server-2019-vs2019:2024.02.21` but _pins_ it, so it doesn't change randomly.
To try and increase confidence that nothing is wrong with Nick's PR. Replacement for #5049 which (for unknown reasons) isn't building cleanly. Fixes a few issues with the router implementation of the signature generation. Mostly related to commas being inserted instead of spaces or vice-versa, but also includes a significant issue where fragments were not being included in the signature. Co-authored-by: [bonnici](https://github.com/bonnici)
Adds a redirect for the previous metrics URL
Co-authored-by: Geoffroy Couprie <apollo@geoffroycouprie.com> Co-authored-by: Gary Pennington <gary@apollographql.com>
Geal
pushed a commit
that referenced
this pull request
May 6, 2024
Fix #5003 This reworks the json schema generation for the config so that it is reduced in size from approx 100k lines to jsut over 7k. The fix involves three things: * Enable references on json schema generation. This got disabled in the past because there were issues with the generated references, but by adding a schema visitor we can work around this. * Adjust the schema generation for Extendable and Conditional. These previously relied on the scheme not using references. * Modify orbiter metrics to redact only based on the properties in the schema rather than on validation metadata as this is not possible when using schema refs: Incomplete basic output Stranger6667/jsonschema-rs#403
Merged
Merged
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.
Fix #5003
This reworks the json schema generation for the config so that it is reduced in size from approx 100k lines to jsut over 7k.
The fix involves three things:
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. ↩