router icon indicating copy to clipboard operation
router copied to clipboard

test(dual-query-planner): improved semantic diff message for parallel nodes

Open duckki opened this issue 1 year ago • 3 comments

Problem

Semantic diff generated messages are not very helpful when the difference is under a parallel node.

Example diff message (old):

mismatched set: missing item[0]
under Parallel node
under item[1]
under Sequence node

Fix

The semantic diff now digs deeper into the parallel nodes if only one child item is mismatching on each side.

For example, JS QP's parallel node has [A, B, C] and Rust QP has [B, C, A']. Then, we can say A and A' are supposed to be matching. Then, the error message includes the difference between A and A'.

Thus, the example diff above improves as following:

mismatch: no matching fragment definition for XXX
under Fetch node (operation name: YYY)
under Flatten node (path: ZZZ)
under item[0]
under Sequence node
under a sole unmatched pair (0 -> 2) in a set comparison
under Parallel node
under item[1]
under Sequence node

Test plan

At this time, we have no known mismatch pattern to test this. But, we can lower the router-bridge version down to 0.6.3+v2.9.2 and run corpus data. One composition ID that shows this difference is ec7ab803-42f1-473b-a9d3-a23524223b21.


Checklist

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

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

Exceptions

Unfortunately, unit testing this is prohibitively cumbersome and integration testing is also tricky since we don't have known mismatches at this time. Since this only affects logging messages, I think it's acceptable.

duckki avatar Oct 22 '24 03:10 duckki

✅ Docs Preview Ready

No new or changed pages found.

svc-apollo-docs avatar Oct 22 '24 03:10 svc-apollo-docs

@duckki, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

github-actions[bot] avatar Oct 22 '24 03:10 github-actions[bot]

CI performance tests

  • [ ] connectors-const - Connectors stress test that runs with a constant number of users
  • [x] const - Basic stress test that runs with a constant number of users
  • [ ] demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
  • [ ] demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
  • [ ] enhanced-signature - Enhanced signature enabled
  • [ ] events - Stress test for events with a lot of users and deduplication ENABLED
  • [ ] 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_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_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • [ ] events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • [ ] events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • [ ] extended-reference-mode - Extended reference mode enabled
  • [ ] large-request - Stress test with a 1 MB request payload
  • [ ] no-tracing - Basic stress test, no tracing
  • [ ] reload - Reload test over a long period of time at a constant rate of users
  • [ ] step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • [ ] step-local-metrics - Field stats that are generated from the router rather than FTV1
  • [ ] step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
  • [x] step - Basic stress test that steps up the number of users over time
  • [ ] xlarge-request - Stress test with 10 MB request payload
  • [ ] xxlarge-request - Stress test with 100 MB request payload

router-perf[bot] avatar Oct 22 '24 04:10 router-perf[bot]