Skip to content

fix(query): sort api_v3 GetDependencies links by parent then child - #9513

Closed
samuel7james wants to merge 1 commit into
jaegertracing:mainfrom
samuel7james:fix/apiv3-getdependencies-sort
Closed

fix(query): sort api_v3 GetDependencies links by parent then child#9513
samuel7james wants to merge 1 commit into
jaegertracing:mainfrom
samuel7james:fix/apiv3-getdependencies-sort

Conversation

@samuel7james

@samuel7james samuel7james commented Sep 7, 2026

Copy link
Copy Markdown

Which problem is this PR solving?

Description of the changes

How was this change tested?

  • Added TestGetDependencies_SortedByParentThenChild, which feeds the mock storage layer three links in an order that isn't recoverable by insertion order or by CallCount, so it only passes if the handler actually sorts.
  • Confirmed the new test fails without the fix (reverted the source change locally, re-ran — it failed with the expected mismatch) and passes with it.
  • go test ./cmd/jaeger/internal/extension/jaegerquery/internal/apiv3/... — all pass, including the pre-existing TestGetDependencies, TestGetDependenciesStorageError, and TestHTTPGateway snapshot tests in that package.
  • golangci-lint run ./cmd/jaeger/internal/extension/jaegerquery/internal/apiv3/... — 0 issues.
  • gofmt -l and go vet on both changed files — clean.
  • I ran these scoped to the changed package rather than the full make lint test, since the change itself only touches this one file pair.

Checklist

AI Usage in this PR (choose one)

See AI Usage Policy.

  • None: No AI tools were used in creating this PR
  • Light: AI provided minor assistance (formatting, simple suggestions)
  • Moderate: AI helped with code generation or debugging specific parts
  • Heavy: AI generated most or all of the code changes

QueryService.GetDependencies builds its result from a map, so
apiv3.Handler.GetDependencies returned the same links in a different
order on repeated identical calls. The legacy HTTP handler (jaegertracing#9119) and
the MCP get_service_dependencies tool (jaegertracing#8403) already sort their
dependency links this way; the gRPC api_v3 path was the one place still
unsorted.

Sorts by Parent then Child with slices.SortFunc/cmp.Compare, matching
the other two paths.

Signed-off-by: Samuel James <samuel7james@gmail.com>
@samuel7james
samuel7james requested a review from a team as a code owner September 7, 2026 15:09
@github-actions github-actions Bot added the pr-quota-reached Tags PRs that exceed the PR limits for new contributors label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Hi @samuel7james, thanks for your contribution! To ensure quality reviews, we limit how many concurrent PRs new contributors can open:

  • Open: 2
  • Limit: 1

This PR is currently on hold. We will automatically move this into the review queue once your existing PRs are merged or closed.

Please see our Contributing Guidelines for details on our tiered quota policy.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The focused implementation matches the issue requirements and includes effective regression coverage.

Pull request overview

Ensures deterministic api_v3 dependency responses by sorting links by parent, then child.

Changes:

  • Adds canonical dependency sorting.
  • Adds regression coverage for unsorted storage results.
File summaries
File Description
grpc_handler.go Sorts dependency links before response conversion.
grpc_handler_test.go Verifies parent/child ordering.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@yurishkuro

Copy link
Copy Markdown
Member

there is already an identical earlier PR #9127

@github-actions github-actions Bot added the waiting-for-author PR is waiting for author to respond to maintainer's comments label Sep 8, 2026
@samuel7james

Copy link
Copy Markdown
Author

Closing — you're right, this duplicates #9127, which was already open. Apologies for not checking for an existing PR before starting. #9127 also has your architectural feedback to work from (sorting belongs in QueryService, not duplicated per-transport), which is the better place to continue this fix.

@github-actions github-actions Bot removed the waiting-for-author PR is waiting for author to respond to maintainer's comments label Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog:bugfix-or-minor-feature pr-quota-reached Tags PRs that exceed the PR limits for new contributors

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: api_v3 GetDependencies returns dependency links in nondeterministic order

4 participants