-
Notifications
You must be signed in to change notification settings - Fork 13
Adds initial otel spec documents #32
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
base: main
Are you sure you want to change the base?
Conversation
spec/registry.yaml
Outdated
brief: "The path of the selection that is being resolved." | ||
type: string | ||
stability: development | ||
examples: ["/persons/0/address", "person[0].address"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to settle here on 1 format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the only mention of "path" in the spec defines it as a heterogenous array of strings and integers, and that JSON is documented as the most common serialization format, I'm inclined to prefer the bracket notation option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, @PascalSenn! 🙇
For persisted operations with a specified operation name, instrumentations MAY provide | ||
a configuration option to enable a more descriptive span name following | ||
`{graphql.operation.type} {graphql.operation.name}` format when | ||
`graphql.operation.name` is available and the operation is successfully identified | ||
in the document. | ||
|
||
> **Warning** | ||
> The `graphql.operation.name` value is provided by the client and can have high | ||
> cardinality. Using it in the GraphQL server span name is NOT RECOMMENDED for | ||
> ad-hoc operations without careful consideration of cardinality implications. | ||
> For persisted operations, the cardinality is bounded and using the operation | ||
> name in the span name is more acceptable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For persisted operations with a specified operation name, instrumentations MAY provide | |
a configuration option to enable a more descriptive span name following | |
`{graphql.operation.type} {graphql.operation.name}` format when | |
`graphql.operation.name` is available and the operation is successfully identified | |
in the document. | |
> **Warning** | |
> The `graphql.operation.name` value is provided by the client and can have high | |
> cardinality. Using it in the GraphQL server span name is NOT RECOMMENDED for | |
> ad-hoc operations without careful consideration of cardinality implications. | |
> For persisted operations, the cardinality is bounded and using the operation | |
> name in the span name is more acceptable. | |
For operation domains with bounded cardinality, instrumentations MAY provide | |
a configuration option to enable a more descriptive span name following the | |
`{graphql.operation.type} {graphql.operation.name}` format when | |
`graphql.operation.name` is available and the operation is successfully identified | |
in the document. | |
> **Warning** | |
> The `graphql.operation.name` value is provided by the client and can have high | |
> cardinality. Using it in the GraphQL server span name is NOT RECOMMENDED for | |
> ad-hoc operations without careful consideration of cardinality implications. | |
> For persisted operations or internal GraphQL APIs, the cardinality is bounded and using the operation | |
> name in the span name is more acceptable. |
I think it's clearer to focus on the condition causing the risk (cardinality), and use "persisted operations" only only exemplify, instead of leading with "persisted operations", as one could interpret "persisted operations" as being the main deciding factor that applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude automated persisted queries
spec/registry.yaml
Outdated
brief: "The path of the selection that is being resolved." | ||
type: string | ||
stability: development | ||
examples: ["/persons/0/address", "person[0].address"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the only mention of "path" in the spec defines it as a heterogenous array of strings and integers, and that JSON is documented as the most common serialization format, I'm inclined to prefer the bracket notation option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting to see this come together!
stability: development | ||
examples: ["query", "mutation", "subscription"] | ||
- id: graphql.operation.hash | ||
brief: "The hash of the operation document." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a server was to generate a normalized hash/signature (example), do you imagine that would be a separate attribute? Is that worth mentioning? At first glance I took this to be a hash of the original raw document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From [RFC] Add Appendix A: Persisted Documents, the sentiment so far is that the hash is created from the raw document, as even small modifications (e.g. removing whitespaces) can change error locations and confuse clients.
That being said, there's quite a bit being discussed in that RFC (e.g. what exactly the hash prefix, the part before :
, means and its semantics) that it makes me think that we can either: follow that "Persisted Documents RFC"s guidance in its current state; delay the introduction of fields related to persisted queries until that RFC is published or at least closer to being published.
Regarding the Operation Signature, I think it is a useful data point but I'm not sure if we want to commit to adding this concept at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fair and is less of a concern to internal APIs. Using the original document makes sense.
For us, we have two hashes but that doesn't mean we need to have it in a spec.
Co-authored-by: Marco Costa <[email protected]>
spec/registry.yaml
Outdated
examples: ["/persons/0/address", "person[0].address"] | ||
note: > | ||
The path represents the location of the field being resolved within the result structure. | ||
Path format may vary between implementations but SHOULD be consistent within an implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path format may vary between implementations but SHOULD be consistent within an implementation. |
For persisted operations with a specified operation name, instrumentations MAY provide | ||
a configuration option to enable a more descriptive span name following | ||
`{graphql.operation.type} {graphql.operation.name}` format when | ||
`graphql.operation.name` is available and the operation is successfully identified | ||
in the document. | ||
|
||
> **Warning** | ||
> The `graphql.operation.name` value is provided by the client and can have high | ||
> cardinality. Using it in the GraphQL server span name is NOT RECOMMENDED for | ||
> ad-hoc operations without careful consideration of cardinality implications. | ||
> For persisted operations, the cardinality is bounded and using the operation | ||
> name in the span name is more acceptable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude automated persisted queries
stability: development | ||
examples: ["person[0].address"] | ||
note: > | ||
The path represents the location of the field being resolved within the result structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add the precision here about field aliasing?
It is implicit that the path should always use aliased field name if any, but should we make it explicit ?
The path represents the location of the field being resolved within the result structure. | |
The path represents the location of the field being resolved within the result structure. If a field in the path has an alias, it SHOULD be used, otherwise the field name is used. |
@@ -0,0 +1,177 @@ | |||
groups: | |||
- id: span.graphql.server.request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the description, we are talking about a Graphql Operation
.
Should the id be span.graphql.server.operation
instead of request
? It would also help to avoid the confusion between the transport (an HTTP request for example) and the actual operation.
stability: development | ||
span_kind: server | ||
brief: > | ||
This span represents an incoming GraphQL request on a server implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, if you agree with the previous comment.
This span represents an incoming GraphQL request on a server implementation. | |
This span represents an incoming GraphQL operation on a server implementation. |
conditionally_required: If parsing succeeded | ||
- ref: graphql.operation.name | ||
requirement_level: | ||
conditionally_required: If available and not empty. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means it is optional (but allowed) to include this attribute when the operation name is not available in the document?
No description provided.