-
Notifications
You must be signed in to change notification settings - Fork 182
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
Allowance for using Stored Operation IDs for Semantic conventions for GraphQL Server #1011
Comments
What a great start to the discussion, thanks! ❤️ I'd like to offer you my perspective and say yeah, I think it is entirely reasonable to include the query hash in the span name, though with a preference to the OperationName. Much like with REST, and including the resource name as the span name, eg Both could work, as in to put the query name in the span name, with its id as an attribute or the inverse.
Though can say, I do prefer the query name as the span name, so a quick glance I know what is going on. |
Just a thought, but why not both? |
@open-telemetry/technical-committee can you transfer this issue to https://github.com/open-telemetry/semantic-conventions? thx |
I am not sure this needs to be moved, as it seems to be a general question about the span name concept. The span name is not supposed to uniquely identify a code location (although in principle, it can). If you want that, there are code.* attributes
|
hey @magicmark! just fyi I have created an issue to suggest working on better GraphQL semantic conventions - and linked your interesting use case: #182 Feel free to add feedback in there as well! |
What are you trying to achieve?
As an alternative to using the GraphQL query name as the span name, I'd like to use its hash (stored operation ID) instead.
Context
We're looking at aligning our GraphQL infra with the following convention:
However, we use document IDs (a sha256 of the query) to uniquely identify queries across our infra. Our org does not allow arbitrary queries at all - every graphql request is known ahead of time, and the query will live somewhere in our codebase.
Why does this matter in the context of the span name?
Query names are not unique (different queries can be defined with the same name across different platforms, or even older/multiple versions of the same query) leading to confusion when trying to find the query in source code.
My concern is this:
Imagine a situation where one of our oncall engineers (who is unfamiliar with the GQL stack) is trying to diagnose a slow or failing operation - they might reasonably use the operation name (query name) as the lookup key when trying to find the query in our codebase to debug further. As mentioned above, this is very susceptible to finding the wrong thing.
"Just add the operation ID as a tag/attribute"
While it is possible to do this, there's still a footgun here in that folks have to know to click into the sapn to copy the operation ID instead of the span name.
Generally we try and build a path for folks to guide them to do the right thing by default - and the "right thing by default" here would be putting the operation name as the span name imo.
The ask
For GraphQL Servers that respect operation IDs only, the span name guidance could allow for using this instead?
FAQs
What about batching
This is actually a different but related point: the first span to our GQL server will be an HTTP request containing multiple queries
https://www.apollographql.com/blog/apollo-client/performance/query-batching
In a case where a server supports batching, I would propose this (where the span name is inside the square brackets)
What about Automatic Persisted Queries?
Apollo support Automatic Persisted Queries (APQ) which is a sort of hybrid. The server supports arbitrary queries, with the query name being the correct and only way to identify them. But subsequent requests have their raw query bodies substituted for IDs instead (which the server has stored in a lookup table). My understanding is that in this setup, IDs are not intended to be used as globally unique identifiers - and more similar to a cache key instead.
tl;dr servers using Apollo APQ should keep the status quo.
The text was updated successfully, but these errors were encountered: