Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer: ftv1 support #1600

Closed
o0Ignition0o opened this issue Aug 24, 2022 · 4 comments · Fixed by #2190
Closed

Defer: ftv1 support #1600

o0Ignition0o opened this issue Aug 24, 2022 · 4 comments · Fixed by #2190
Assignees

Comments

@o0Ignition0o
Copy link
Contributor

o0Ignition0o commented Aug 24, 2022

Followup to #1599 where i disabled sending traces for defered queries, we want to have a look at what needs to be done so we can support it

@prasek
Copy link
Contributor

prasek commented Oct 20, 2022

will this fix the missing trace data in Studio?
image

@timbotnik
Copy link
Contributor

timbotnik commented Nov 9, 2022

Ideally we can come up with a solution that is backwards-compatible with the current protobuf protocol, in the sense that for a "good enough" experience we can think of deferred fetches as simply additional nodes in the trace. I'd like to vet that assumption as I've heard mention of things like modifying the protobuf to include finer-grained defer semantics, and it would be great to avoid that if we can in the short-term.

@timbotnik
Copy link
Contributor

timbotnik commented Nov 11, 2022

As a response to the above thought, a hot take from @glasser:

The query planner produces a query plan which is a tree shape with various node types like Fetch, Parallel, Sequence, Flatten. This is actually a user-visible thing — eg Explorer will show a graph version of the plan. The usage reporting protocol represents supergraph traces with a structure that reflects the query plan precisely (with any field execution traces from subgraphs dangling off of the appropriate Fetch nodes in the tree). The trace viewer happens to squash this into a flat list of Fetch nodes but that's honestly a real shame, and it's great that we have the full tree structure in traces so that once we get around to improving the viewer old traces will immediately show the correct structure.

The defer project added a Defer node to the kinds of nodes that can exist in a query plan (which, again, is a very user-visible thing). I suppose one technically could choose to change "the trace's query plan represents the query plan" into "the trace's query plan represents a mangled version of the query plan that for some reason doesn't have a couple kinds of nodes in it" but I can't imagine why you'd do that. It's just adding a couple lines to the protobuf definition that match the existing query planner definition pretty much precisely.

You're welcome to implement this in a lossy hacky way but if you do so I'll just spend half an hour throwing together the 20 line PR to do it the right way and then you can choose which version to merge :)

To be clear, I'm talking specifically about "should the query plan part of the trace that we store in our storage reflect the query plan or reflect something that's sort of like the query plan but not really". I'm not talking about "should the trace viewer in Studio show the actual query plan or just a flat list of fetch nodes". I think the answer to the second question is probably yes too but that is the next bullet point (and requires a bit of actual design). This bullet point basically just requires mindless copying of a structure that already exists in the query planner into the report format, and updating a couple places that process the various kinds of nodes to know that a new kind exists.

BrynCooke pushed a commit that referenced this issue Dec 1, 2022
You can now view traces in Apollo Studio as normal.

Fixes #1600
BrynCooke pushed a commit that referenced this issue Dec 1, 2022
You can now view traces in Apollo Studio as normal.

Fixes #1600
BrynCooke pushed a commit that referenced this issue Dec 2, 2022
You can now view traces in Apollo Studio as normal.

Fixes #1600
BrynCooke added a commit that referenced this issue Dec 2, 2022
You can now view traces in Apollo Studio as normal.
Also improved testing and fixed missing variables in Apollo tracing.
Fixes #1600 #2186
 

<!--
First, 🌠 thank you 🌠 for considering a contribution to Apollo!

Some of this information is also included in the /CONTRIBUTING.md file
at the
root of this repository.  We suggest you read it!

  https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md

Here are some important details to keep in mind:

* ⏰ Your time is important
To save your precious time, if the contribution you are making will
take more than an hour, please make sure it has been discussed in an
        issue first. This is especially true for feature requests!

* 💡 Features
Feature requests can be created and discussed within a GitHub Issue.
Be sure to search for existing feature requests (and related issues!)
prior to opening a new request. If an existing issue covers the need,
please upvote that issue by using the 👍 emote, rather than opening a
        new issue.

* 🕷 Bug fixes
These can be created and discussed in this repository. When fixing a
bug,
please _try_ to add a test which verifies the fix. If you cannot, you
should
still submit the PR but we may still ask you (and help you!) to create a
test.

* 📖 Contribution guidelines
Follow https://github.com/apollographql/router/blob/HEAD/CONTRIBUTING.md
when submitting a pull request. Make sure existing tests still pass, and
add
        tests for all new behavior.

* ✏️ Explain your pull request
Describe the big picture of your changes here to communicate to what
        your pull request is meant to accomplish. Provide 🔗 links 🔗 to
associated issues! Documentation in the docs/ directory should be
updated
        as necessary.  Finally, a /CHANGELOG.md entry should be added.

We hope you will find this to be a positive experience! Contribution can
be
intimidating and we hope to alleviate that pain as much as possible.
Without
following these guidelines, you may be missing context that can help you
succeed
with your contribution, which is why we encourage discussion first.
Ultimately,
there is no guarantee that we will be able to merge your pull-request,
but by
following these guidelines we can try to avoid disappointment.

-->

Co-authored-by: bryn <[email protected]>
@BrynCooke
Copy link
Contributor

The protobuf should be fully populated now.

image

@BrynCooke BrynCooke modified the milestones: v1-NEXT, v1.5.0 Dec 2, 2022
@garypen garypen added this to the v1.5.0 milestone Dec 5, 2022
@BrynCooke BrynCooke modified the milestone: v1.5.0 Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants