adds operation planning span for distributed graphql #34
Conversation
| The coordinate follows the format "{ParentType}.{fieldName}" and uniquely | ||
| identifies the field within the GraphQL schema. | ||
| - id: graphql.operation.step.id | ||
| brieg: "The id of the step in the execution plan." |
There was a problem hiding this comment.
| brieg: "The id of the step in the execution plan." | |
| brief: "The id of the step in the execution plan." |
| identifies the field within the GraphQL schema. | ||
| - id: graphql.operation.step.id | ||
| brieg: "The id of the step in the execution plan." | ||
| type: int |
There was a problem hiding this comment.
Must the id be an integer? Grafast currently uses numeric step IDs but it reserves space for IDs to be strings in future. Strings allow for more stable plans, since a small change won't need to re-number every step - and the id can include details of what it's doing like NullCheck-1 or whatever. Also, if your ids are numbers then parsing them from string should be straightforward, but the other way around is more complex.
| type: int | |
| type: string |
| - ref: graphql.operation.name | ||
| requirement_level: recommended | ||
| - ref: graphql.operation.hash | ||
| requirement_level: recommended |
There was a problem hiding this comment.
Is there a reason the graphql.operation.id attribute was omitted for this span? I'm not trying to advocate for it, I'm just wondering why other internal spans have it but this one does not.
The same applies to the span.graphql.server.operation.step.execution span.
| The step execution phase is where the actual data retrieval and field resolution | ||
| occurs, making it critical for performance monitoring and debugging. |
There was a problem hiding this comment.
Sorry for my lack of experience with distributed request execution, but I'd like to make sure that this span's definition is clear: How does this span relate to span.graphql.server.operation.execution in a distributed request execution? Is one normally the parent of the other? Are they normally mutually exclusive?
There was a problem hiding this comment.
I would say that span.graphql.server.operation.step.execution should always be a descendant of an span.graphql.server.operation.execution span.
I think that in most cases, the span.graphql.server.operation.planning span would also be a descendant of span.graphql.server.operation.execution, but perhaps that is more dependent of the specific implementation of the gateway.
You can see Query Planning + Planning Execution as a replacement of the normal Graphql Operation Execution. It just replace the classic resolver based execution logic by another approach, usually more optimal when field resolution can be deeply batched together for each data source.
| requirement_level: recommended | ||
| - ref: graphql.operation.hash | ||
| requirement_level: recommended | ||
| - ref: graphql.operation.step.id |
There was a problem hiding this comment.
We also need a plan id and a step kind
There was a problem hiding this comment.
It would also be useful to have an attribute to indicate the data source target (usually a subgraph).
| - ref: graphql.operation.hash | ||
| requirement_level: recommended | ||
|
|
||
| - id: span.graphql.server.operation.step.execution |
There was a problem hiding this comment.
I think I would find it more obvious what it represent if the name contained the word plan or something like this, what do you think ?
| - id: span.graphql.server.operation.step.execution | |
| - id: span.graphql.server.operation.plan.step |
There was a problem hiding this comment.
For Grafast we have two phases: planning and execution. plan.step would suggest time taken to plan a step. What we care about here is really executing the step. I do think the heirarchy is the wrong way around though - span.graphql.server.operation.execute.step; though as I've mentioned before this seems super redundant since execution only over applies to an operation on the "service", so I'd just do span.graphql.execute.step or span.graphql.execute.plan.step
There was a problem hiding this comment.
Yes I agree, the last one seems good to me :-)
| brief: > | ||
| This span represents the execution of an individual step within a GraphQL operation execution plan. | ||
| note: | | ||
| **Span name** SHOULD be `GraphQL Step Execution`. |
There was a problem hiding this comment.
Same here, I would add the word Plan to make it clear that this is not about normal GraphQL execution.
| **Span name** SHOULD be `GraphQL Step Execution`. | |
| **Span name** SHOULD be `GraphQL Plan Step Execution`. |
| This span covers the execution of a single step in the GraphQL execution plan, | ||
| where each step typically represents the resolution of one or more fields | ||
| that can be executed together. In federated GraphQL systems, a step often |
There was a problem hiding this comment.
Nitpicking: I think we resolved here is a better feat. The query plan can also involve other sources than GraphQL (like a database or something similar) where the world executed would not have much sense.
| that can be executed together. In federated GraphQL systems, a step often | |
| that can be resolved together. In federated GraphQL systems, a step often |
There was a problem hiding this comment.
But the graphql spec talks about field execution :)
5 participants
No description provided.