Router generation of sig and refs for apollo reporting #4796

bonnici · 2024-03-13T06:27:58Z

This PR introduces experimental support for generating the stats report key (signature) and referenced fields in native rust code instead of using what is generated by router-bridge as part of query planning. There were a number of gotchas that I found in the JS code that I have faithfully ported across, but at this point I've run enough unit tests and fuzz testing that I'm confident it's generating the same as the JS code.

Any comments related to Rust code conventions etc are very welcome as I haven't written much Rust code before.

Some specific questions I have:

Should I keep this in a separate crate or move the functionality to a public function within the router crate? It was originally moved to a separate crate because we thought it would be necessary to be able to run the fuzz tests, but we should be able to do that with a public function.
Are the auto-generated updates to the licence ok? I'm not sure how to validate that
Is there anything else I should test? Should I try to do some benchmarking? Should I run this through our corpus tests?

Implements https://apollographql.atlassian.net/browse/FED-117.

Checklist

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

Exceptions

Note any exceptions here

Notes

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. ↩

router-perf · 2024-03-13T06:28:29Z

lrlna · 2024-03-13T10:41:07Z

apollo-router/src/query_planner/bridge_query_planner.rs

+        format!("# {}\n{}", operation_name, stripped_body)
+    }
+
+    // todo move this somewhere else? somewhere where it's possible to do fuzzing?


perhaps the operation signature can just live in its own small crate in the router workspace? we don't have to publish it. it would allow this to be called from the fuzz crate in the router.

@Geal were there any previous discussions in the router that would not allow for there to be a new workspace crate?

We do publish https://crates.io/crates/apollo-router for Rust plugins, so its dependencies need to be published as well. Would https://github.com/apollographql/federation-next/ / https://crates.io/crates/apollo-federation be a good place for this?

I don't think this functionality belongs with federation at all, it feels so much closer to how the router itself interops with studio. At that point I'd rather have this as a published crate in the router's repository?

That follows the pattern of the JS libraries too. The functionality I'm porting in this PR is in the https://github.com/apollographql/apollo-utils repo, not the federation repo.

@lrlna I don't think I can put this code into a separate crate. It has dependencies on some classes/structs in the router crate like ParsedDocument and ReferencedFieldsForType so if I used it in its own crate I think there would be a circular dependency between apollo-router and this new interop crate?

If I just move the code to a public function in the apollo-router crate, can I set up the fuzzer to call that?

It took me a little while to figure it out how to do that but I've done it now and I ran into a very odd dependency issue. When I try to run the fuzzer using

cargo +nightly fuzz run apollo_router_studio_interop

I get the error

error[E0635]: unknown feature `stdsimd` --> /Users/nickmarsh/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ahash-0.8.6/src/lib.rs:99:42 | 99 | #![cfg_attr(feature = "stdsimd", feature(stdsimd))]

Apparently this is because of a transitive dependency to ahash and we'd need to upgrade from v0.8.6 to v0.8.7, but the dependency chain is very complicated. Any thoughts on how I can resolve this?

Dependency chain

❯ cargo tree -i ahash ahash v0.8.6 ├── hashbrown v0.14.1 │ ├── dashmap v5.5.3 │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ └── serial_test v3.0.0 │ │ [dev-dependencies] │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ ├── indexmap v2.2.3 │ │ ├── apollo-compiler v1.0.0-beta.13 │ │ │ ├── apollo-federation v0.0.8 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ ├── apollo-federation v0.0.8 (*) │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ ├── h2 v0.3.24 │ │ │ ├── aws-smithy-runtime v1.1.6 │ │ │ │ ├── aws-config v1.1.6 │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── aws-sdk-sso v1.14.0 │ │ │ │ │ └── aws-config v1.1.6 (*) │ │ │ │ ├── aws-sdk-ssooidc v1.14.0 │ │ │ │ │ └── aws-config v1.1.6 (*) │ │ │ │ └── aws-sdk-sts v1.14.0 │ │ │ │ └── aws-config v1.1.6 (*) │ │ │ ├── hyper v0.14.28 │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── aws-config v1.1.6 (*) │ │ │ │ ├── aws-smithy-runtime v1.1.6 (*) │ │ │ │ ├── axum v0.6.20 │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ └── tonic v0.9.2 │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── opentelemetry-otlp v0.13.0 │ │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ └── opentelemetry-proto v0.3.0 │ │ │ │ │ └── opentelemetry-otlp v0.13.0 (*) │ │ │ │ │ [dev-dependencies] │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── hyper-rustls v0.24.2 │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── aws-smithy-runtime v1.1.6 (*) │ │ │ │ │ └── reqwest v0.11.24 │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── opentelemetry-datadog v0.8.0 │ │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── opentelemetry-http v0.9.0 │ │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ │ ├── opentelemetry-datadog v0.8.0 (*) │ │ │ │ │ │ ├── opentelemetry-jaeger v0.19.0 │ │ │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ │ ├── opentelemetry-otlp v0.13.0 (*) │ │ │ │ │ │ └── opentelemetry-zipkin v0.18.0 │ │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── opentelemetry-jaeger v0.19.0 (*) │ │ │ │ │ ├── opentelemetry-otlp v0.13.0 (*) │ │ │ │ │ └── opentelemetry-zipkin v0.18.0 (*) │ │ │ │ │ [dev-dependencies] │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── hyper-timeout v0.4.1 │ │ │ │ │ └── tonic v0.9.2 (*) │ │ │ │ ├── hyperlocal v0.8.0 │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ [dev-dependencies] │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── reqwest v0.11.24 (*) │ │ │ │ ├── tonic v0.9.2 (*) │ │ │ │ └── wiremock v0.5.22 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ [dev-dependencies] │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── reqwest v0.11.24 (*) │ │ │ └── tonic v0.9.2 (*) │ │ ├── petgraph v0.6.4 │ │ │ ├── apollo-federation v0.0.8 (*) │ │ │ └── daggy v0.8.0 │ │ │ └── test-span v0.7.0 │ │ │ [dev-dependencies] │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ ├── serde_json v1.0.114 │ │ │ ├── access-json v0.1.0 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── apollo-federation v0.0.8 (*) │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── assert-json-diff v2.0.2 │ │ │ │ └── wiremock v0.5.22 (*) │ │ │ ├── axum v0.6.20 (*) │ │ │ ├── deno_core v0.200.0 │ │ │ │ ├── deno_console v0.115.0 │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ [build-dependencies] │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ ├── deno_crypto v0.129.0 │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ │ [build-dependencies] │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ ├── deno_url v0.115.0 │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ │ [build-dependencies] │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ ├── deno_web v0.146.0 │ │ │ │ │ ├── deno_crypto v0.129.0 (*) │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ │ [build-dependencies] │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ ├── deno_webidl v0.115.0 │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ │ [build-dependencies] │ │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ │ [build-dependencies] │ │ │ │ └── router-bridge v0.5.16+v2.7.1 (*) │ │ │ ├── graphql_client v0.13.0 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── http-types v2.12.0 │ │ │ │ └── wiremock v0.5.22 (*) │ │ │ ├── jsonpath-rust v0.3.5 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── jsonpath_lib v0.3.0 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── jsonschema v0.17.1 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── jsonwebtoken v9.2.0 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── opentelemetry-stdout v0.1.0 │ │ │ │ [dev-dependencies] │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── opentelemetry-zipkin v0.18.0 (*) │ │ │ ├── opentelemetry_sdk v0.20.0 │ │ │ │ ├── opentelemetry v0.20.0 │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ ├── opentelemetry-datadog v0.8.0 (*) │ │ │ │ │ ├── opentelemetry-jaeger v0.19.0 (*) │ │ │ │ │ ├── opentelemetry-semantic-conventions v0.12.0 │ │ │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ │ ├── opentelemetry-datadog v0.8.0 (*) │ │ │ │ │ │ ├── opentelemetry-jaeger v0.19.0 (*) │ │ │ │ │ │ ├── opentelemetry-otlp v0.13.0 (*) │ │ │ │ │ │ └── opentelemetry-zipkin v0.18.0 (*) │ │ │ │ │ ├── opentelemetry-zipkin v0.18.0 (*) │ │ │ │ │ └── tracing-opentelemetry v0.21.0 │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ │ [dev-dependencies] │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── opentelemetry-otlp v0.13.0 (*) │ │ │ │ ├── opentelemetry-prometheus v0.13.0 │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── opentelemetry-proto v0.3.0 (*) │ │ │ │ ├── opentelemetry-stdout v0.1.0 (*) │ │ │ │ └── tracing-opentelemetry v0.21.0 (*) │ │ │ ├── proteus v0.5.0 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── reqwest v0.11.24 (*) │ │ │ ├── router-bridge v0.5.16+v2.7.1 (*) │ │ │ ├── schemars v0.8.16 │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── serde_json_bytes v0.2.2 │ │ │ │ ├── apollo-compiler v1.0.0-beta.13 (*) │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ ├── sourcemap v6.4.1 │ │ │ │ └── deno_core v0.200.0 (*) │ │ │ ├── test-span v0.7.0 (*) │ │ │ ├── tracing-subscriber v0.3.18 │ │ │ │ ├── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── test-log v0.2.14 │ │ │ │ │ [dev-dependencies] │ │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ ├── test-span v0.7.0 (*) │ │ │ │ ├── tracing-opentelemetry v0.21.0 (*) │ │ │ │ └── tracing-test v0.2.4 │ │ │ │ [dev-dependencies] │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ │ [dev-dependencies] │ │ │ │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) │ │ │ └── wiremock v0.5.22 (*) │ │ └── serde_json_bytes v0.2.2 (*) │ └── lru v0.12.2 │ └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) ├── jsonschema v0.17.1 (*) └── rhai v1.17.1 └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router) [dev-dependencies] └── apollo-router v1.43.0 (/Users/nickmarsh/src/router/apollo-router)

@bonnici It looks like it will work with just updating the deps here:

cargo update -p ahash cargo update -p curve25519-dalek@4.0.0

(somehow that other dependency has a similar issue)

@Geal Thank you for that but unfortunately I ran into yet another strange issue I have no idea how to fix. The cargo update -p commands got me past the unknown feature stdsimd error but now I'm getting an even stranger linker error:

error: linking with `cc` failed: exit status: 1 | = note: env -u IPHONEOS_DEPLOYMENT_TARGET -u TVOS_DEPLOYMENT_TARGET LC_ALL="C" PATH="/Users/nickmarsh/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/bin:/opt/homebrew/opt/curl/bin:/Users/nickmarsh/google-cloud-sdk/bin:/Users/nickmarsh/.volta/bin:/opt/homebrew/bin:/opt/homebrew/sbin:/usr/local/bin:/System/Cryptexes/App/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/local/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/bin:/var/run/com.apple.security.cryptexd/codex.system/bootstrap/usr/appleinternal/bin:/Users/nickmarsh/.cargo/bin:/Users/nickmarsh/.rover/bin:/Users/nickmarsh/Library/Application Support/JetBrains/Toolbox/scripts" VSLANG="1033" ZERO_AR_DATE="1" "cc" "-arch" "arm64" "/var/folders/q2/y4vh72_9197819gxrzgwtx3r0000gn/T/rustcQOdT9N/symbols.o" "-Wl,-rpath" "-Xlinker" "/Users/nickmarsh/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib" "-lrustc-nightly_rt.asan" "/Users/nickmarsh/src/router/target/aarch64-apple-darwin/release/deps/apollo_router_studio_interop-38c9c488e24b25f8.apollo_router_studio_interop.a0f57017173ec7f8-cgu.0.rcgu.o" "/Users/nickmarsh/src/router/target/aarch64-apple-darwin/release/deps/apollo_router_studio_interop-38c9c488e24b25f8.1euljvum1py0sn1w.rcgu.o" "-L" < huge list of folders goes here > "-lc++" "-framework" "SystemConfiguration" "-framework" "Security" "-framework" "CoreFoundation" "-framework" "Security" "-liconv" "-lSystem" "-lc" "-lm" "-L" "/Users/nickmarsh/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib" "-o" "/Users/nickmarsh/src/router/target/aarch64-apple-darwin/release/deps/apollo_router_studio_interop-38c9c488e24b25f8" "-nodefaultlibs" = note: ld: warning: search path '/Users/nickmarsh/src/router/target/aarch64-apple-darwin/release/build/libz-ng-sys-2ace1440088f968b/out/lib64' not found ld: initializer pointer has no target in '/Users/nickmarsh/src/router/target/aarch64-apple-darwin/release/deps/libproteus-6844fdb36340bdae.rlib[3](proteus-6844fdb36340bdae.proteus.9e4a919a603b00d1-cgu.0.rcgu.o)' clang: error: linker command failed with exit code 1 (use -v to see invocation) error: could not compile `router-fuzz` (bin "apollo_router_studio_interop") due to 1 previous error Error: failed to build fuzz script: ASAN_OPTIONS="detect_odr_violation=0" RUSTFLAGS="-Cpasses=sancov-module -Cllvm-args=-sanitizer-coverage-level=4 -Cllvm-args=-sanitizer-coverage-inline-8bit-counters -Cllvm-args=-sanitizer-coverage-pc-table -Cllvm-args=-sanitizer-coverage-trace-compares --cfg fuzzing -Clink-dead-code -Zsanitizer=address -Cdebug-assertions -C codegen-units=1" "cargo" "build" "--manifest-path" "/Users/nickmarsh/src/router/fuzz/Cargo.toml" "--target" "aarch64-apple-darwin" "--release" "--config" "profile.release.debug=true" "--bin" "apollo_router_studio_interop"

The fuzz build also started taking ages to finish (15+ minutes) and using up most of my free memory. Any thoughts?

it looks like the fuzzing framework is trying to do too much, while the router's code is too big. So we need to change gears. I left suggestions in the PR that will make it build and run quickly, I just tested. The nice thing with that solution is that we won't need to reexport the module.
You also need to remove this line:

router/fuzz/fuzz_targets/federation.rs

Line 2 in 181e8f7

mod invariant_router;

Meanwhile I have a PR up to build the fuzzers in CI and make sure we don't regress there #4925

Thank you! I had to make a few other changes but it looks like it's all working now.

This reverts commit dae049c.

…piler

SimonSapin · 2024-03-25T16:21:05Z

apollo_router__configuration__tests__schema_generation.snap.new should not be committed to the repository. Instead please run cargo install cargo-insta (once) and cargo insta review (after a snapshot test failures) to review the change to expected output of a snapshot test. In the case of the "configuration schema generation" test, a change is expected when adding a new config key.

Geal · 2024-04-09T12:04:05Z

apollo-router/src/lib.rs

+    // For fuzz testing
+    pub use crate::apollo_studio_interop::generate_usage_reporting;
+    pub use crate::apollo_studio_interop::UsageReportingComparisonResult;


Suggested change

// For fuzz testing

pub use crate::apollo_studio_interop::generate_usage_reporting;

pub use crate::apollo_studio_interop::UsageReportingComparisonResult;

Geal · 2024-04-09T12:04:16Z

fuzz/Cargo.toml

 apollo-smith = { version = "0.5.0", features = ["parser-impl"] }
 apollo-parser = "0.7.6"
+apollo-router = { path = "../apollo-router" }


Suggested change

apollo-router = { path = "../apollo-router" }

Geal · 2024-04-09T12:04:50Z

fuzz/fuzz_targets/apollo_router_studio_interop.rs

+use apollo_router::_private::generate_usage_reporting;
+use apollo_router::_private::UsageReportingComparisonResult;


Suggested change

use apollo_router::_private::generate_usage_reporting;

use apollo_router::_private::UsageReportingComparisonResult;

#[path = "../../apollo-router/src/apollo_studio_interop/mod.rs"]

mod apollo_router_usage_reporting;

use apollo_router_usage_reporting::generate_usage_reporting;

use apollo_router_usage_reporting::UsageReportingComparisonResult;

Geal · 2024-04-10T07:39:49Z

apollo-router/src/query_planner/bridge_query_planner.rs

+                            UsageReportingComparisonResult::StatsReportKeyNotEqual
+                                | UsageReportingComparisonResult::BothNotEqual
+                        ) {
+                            tracing::warn!(


why are the 3 metrics calls (here and at lines 613 and 630) at different log levels?

I used info for the "happy state" and warn for the "unhappy state". Should I be using warn for all the metric traces?

Geal · 2024-04-10T07:43:22Z

apollo-router/src/query_planner/caching_query_planner.rs

@@ -447,6 +447,7 @@ where
                                });
                            }

+                            // This will be overridden when running in ApolloMetricsGenerationMode::New mode


we will be able to do even better: since usage reporting only depends on the query and the schema, we can calculate it way earlier, in the query analysis layer, and then we can apply some query normalization before calling into query planning, which will reduce the number of generated query plans. Example: the same query but with a different operation name will point to the same stored plan

Geal · 2024-04-10T07:44:41Z

apollo-router/src/query_planner/bridge_query_planner.rs

+                        .experimental_apollo_metrics_generation_mode,
+                    ApolloMetricsGenerationMode::New | ApolloMetricsGenerationMode::Both
+                ) {
+                    // If the query is filtered, we want to generate the signature using the original query and generate the


we want to generate the signature from the original query, but do we want to generate the usage reporting from the original too?

In the code above:

// the `statsReportKey` field should match the original query instead of the filtered query, to index them all under the same query

We only regenerate the query, not the referenced fields. I wanted to keep the behaviour of the new process the same as the old process so we can do an equal comparison. But I think ideally we would probably want to just use the original query for both, since that's the operation that was executed and the fields that were referenced. But I thought there might be a reason why the signature was regenerated so didn't want to change that.

Geal · 2024-04-10T07:50:07Z

apollo-router/src/apollo_studio_interop/mod.rs

+}
+
+#[cfg(test)]
+mod tests {


nit: could you split the tests in a separate file?

I tried a few ways of doing this but the tests wouldn't run in any attempt I made, can you see what I've done wrong in the latest commit?

Geal · 2024-04-10T09:02:58Z

apollo-router/src/apollo_studio_interop/mod.rs

+                let mut sorted_self_field_names = self_refs.field_names.clone();
+                sorted_self_field_names.sort();
+                let mut sorted_other_field_names = other_refs.field_names.clone();
+                sorted_other_field_names.sort();
+
+                if sorted_self_field_names != sorted_other_field_names {
+                    return false;
+                }


if there's a lot of fields, it might be faster to collect the names in a HashSet and then check that they are equal

Right. I think I did it this way because originally I had it so the order mattered. I'll update this.

Geal · 2024-04-10T09:14:52Z

apollo-router/src/apollo_studio_interop/mod.rs

+        sorted_fragments.sort_by_key(|&(k, _)| k);
+
+        sorted_fragments.into_iter().for_each(|(_, f)| {
+            result.push_str(&ApolloReportingSignatureFormatter::Fragment(f).to_string())


Suggested change

result.push_str(&ApolloReportingSignatureFormatter::Fragment(f).to_string())

write!(&mut result, "{}", ApolloReportingSignatureFormatter::Fragment(f));

that results in less allocations overall

This one didn't work for me - I got the error cannot write into &mut std::string::String

Geal · 2024-04-10T09:16:44Z

apollo-router/src/apollo_studio_interop/mod.rs

+        }
+    }
+
+    fn extract_fields(&mut self, parent_type: &String, selection_set: &SelectionSet) {


Suggested change

fn extract_fields(&mut self, parent_type: &String, selection_set: &SelectionSet) {

fn extract_fields(&mut self, parent_type: &str, selection_set: &SelectionSet) {

The String is forcing you to convert first into an allocated String everywhere, then refer to it by a reference, while the underlying type could directly defer to a &str

(this and other perf stuff could be adressed in a follow up, that's not blocking)

I had to do an .into() a few times after making this change because other functions were expecting a String so I don't know if I actually made it any better or not.

This reverts commit cbc224f.

Geal · 2024-04-12T08:26:56Z

the test_updated CI failure will be fixed once #4945 is merged

bonnici added 3 commits March 13, 2024 12:14

Add config

01ff863

Hooked config into query planner

f2d7fcb

Mock implementations

787e965

Remove temp code

5895566

lrlna reviewed Mar 13, 2024

View reviewed changes

bonnici added 22 commits March 14, 2024 10:35

Add metrics

59d1068

Nearly working implementation of ref generation for a couple of examples

edd56b2

Refactored and fixed refs generation

69bf0cf

WIP signature generation

a6ac6ff

WIP work on signature generation

7203e53

Temp experimenting with visitor

dae049c

Revert "Temp experimenting with visitor"

a1a0c3d

This reverts commit dae049c.

WIP signature generation using string builder pattern

1d8c22e

Refactor to use formatter

afc8ac4

Getting closer

1f63229

All tests working

4e58bcb

Fix some todos

b14265d

move to separate file and depend only on router-bridge and apollo-com…

c69995c

…piler

Move to internal crate

c80c025

Attempt at a fuzzer

69de1c0

Less wrappers

079b15f

More qualified names

ccdb257

Refactor and working on removing regex

f140eb9

Fix formatting and remove regex

0aab7af

Refactor

29cda59

Lint fixes

a1dac41

WIP fuzz debugging

1bf8053

Fix snapshot

c6881c6

bonnici and others added 6 commits March 28, 2024 14:27

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

ceaf317

Revert removal of apparently used types

aeb1555

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

a652d61

Don't report difference for known issue

641851f

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

68df1fc

Update packages and attempt to fix fuzz target

f0fac7f

Geal reviewed Apr 9, 2024

View reviewed changes

bonnici added 5 commits April 10, 2024 11:36

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

d242df0

updated licences

2c2aa7b

Fixes for fuzzer

28bfd91

Changeset update

644851d

Use both mode by default

1bf6013

Geal approved these changes Apr 10, 2024

View reviewed changes

Update snapshot

bb331ee

garypen mentioned this pull request Apr 10, 2024

transient in memory cache #4922

Draft

6 tasks

bonnici and others added 7 commits April 11, 2024 16:09

Merge branch 'apollographql:dev' into njm/P-681/rust-apollo-reporting

f2070bb

Address PR comments

cbc224f

Revert "Address PR comments"

951e6c6

This reverts commit cbc224f.

Re-apply safe change

6e6ee07

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

f95fdde

Fix merge issues

7bef5c9

Fix whitespace

83ad99e

Merge branch 'dev' into njm/P-681/rust-apollo-reporting

7ec9797

Geal enabled auto-merge (squash) April 12, 2024 08:35

Geal merged commit 847d055 into apollographql:dev Apr 12, 2024
12 of 13 checks passed

bonnici mentioned this pull request Apr 15, 2024

Signature and reference generation tweaks #4951

Merged

6 tasks

abernix mentioned this pull request Apr 22, 2024

prep release: v1.45.0 #4995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Router generation of sig and refs for apollo reporting #4796

Router generation of sig and refs for apollo reporting #4796

bonnici commented Mar 13, 2024 •

edited

router-perf bot commented Mar 13, 2024

lrlna Mar 13, 2024

SimonSapin Mar 18, 2024

lrlna Mar 18, 2024

bonnici Mar 19, 2024

bonnici Mar 21, 2024

bonnici Mar 28, 2024 •

edited

Geal Apr 8, 2024

bonnici Apr 9, 2024 •

edited

Geal Apr 9, 2024

bonnici Apr 10, 2024

SimonSapin commented Mar 25, 2024

Geal Apr 9, 2024

Geal Apr 9, 2024

Geal Apr 9, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal Apr 10, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal Apr 10, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal Apr 10, 2024

Geal Apr 10, 2024

bonnici Apr 11, 2024

Geal commented Apr 12, 2024 •

edited

	// For fuzz testing
	pub use crate::apollo_studio_interop::generate_usage_reporting;
	pub use crate::apollo_studio_interop::UsageReportingComparisonResult;

		use apollo_router::_private::generate_usage_reporting;
		use apollo_router::_private::UsageReportingComparisonResult;

-use apollo_router::_private::generate_usage_reporting;
-use apollo_router::_private::UsageReportingComparisonResult;
+#[path = "../../apollo-router/src/apollo_studio_interop/mod.rs"]
+mod apollo_router_usage_reporting;
+use apollo_router_usage_reporting::generate_usage_reporting;
+use apollo_router_usage_reporting::UsageReportingComparisonResult;

	result.push_str(&ApolloReportingSignatureFormatter::Fragment(f).to_string())
	write!(&mut result, "{}", ApolloReportingSignatureFormatter::Fragment(f));

	fn extract_fields(&mut self, parent_type: &String, selection_set: &SelectionSet) {
	fn extract_fields(&mut self, parent_type: &str, selection_set: &SelectionSet) {

Router generation of sig and refs for apollo reporting #4796

Router generation of sig and refs for apollo reporting #4796

Conversation

bonnici commented Mar 13, 2024 • edited

Footnotes

router-perf bot commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonnici Mar 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bonnici Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSapin commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geal commented Apr 12, 2024 • edited

bonnici commented Mar 13, 2024 •

edited

bonnici Mar 28, 2024 •

edited

bonnici Apr 9, 2024 •

edited

Geal commented Apr 12, 2024 •

edited