Graphql should add operation name and hash in span

[Sytten]

Is your feature request related to a problem? Please describe

Currently the name of the operation and the hash are not provided in the attributes of the span. It just prints query, which is not even the operation name but the operation type. This makes it very difficult to find a given operation in all the traces.

Describe the solution you'd like to see

By default the operation name should be provided as operation.name and the current name should be renamed to type.
Ideally the hash would also be provided. Getting a consistant is however difficult (each server implementation is different), so providing a hook for customization would be best with a sensible default.

Describe alternatives you've considered

I considered using envelop instead and skip the plugin from opentelemetry, but this requires significant changes in the code.

Additional context

[dyladan]

Is there a semantic convention for graphql queries yet? If not, it might be worth adding one to the spec

[Sytten]

@dyladan Not sure what you mean by semantic convention?

[dyladan]

Semantic conventions are sets of attributes that standardize how attributes are set for various types of spans. The reason for this is so that if there are multiple instrumented libraries that are tracing the same types of calls (for example you are tracing graphql from 2 different libraries, or even languages), they get the same attributes. This makes it so they can be consistently processed by your tracing backend.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/README.md

It looks like there isn't a semantic convention for graphql, but the database semconv and the rpc semconv might be partially applicable? I think this might be a good case to make an addition to the specification though.

[jord1e]

Semantic conventions are sets of attributes that standardize how attributes are set for various types of spans. The reason for this is so that if there are multiple instrumented libraries that are tracing the same types of calls (for example you are tracing graphql from 2 different libraries, or even languages), they get the same attributes. This makes it so they can be consistently processed by your tracing backend.

open-telemetry/opentelemetry-specification@main/specification/trace/semantic_conventions/README.md

It looks like there isn't a semantic convention for graphql, but the database semconv and the rpc semconv might be partially applicable? I think this might be a good case to make an addition to the specification though.

Who would be the one to start the initiative to add GraphQL conventions to the spec?
As GraphQL is gaining popularity it should be a priority to have uniform telemetry data.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[jord1e]

@github-actions

[dchambers]

@Sytten, is there a particular reason why you think the field should be called operation.name rather than graphql.operation.name, otherwise #847 would seem (to me) to be the right approach? This would hopefully also avoid the need for a naming convention discussion, given that @opentelemetry/instrumentation-graphql already emits a graphql.operation.name field, and fixing it would just be an improvement to what it already does.

[Sytten]

I have no opinion on the name of the field per say, just that the current implementation is wrong. Changing it would be a breaking change in any case.

[jscherer92]

@dyladan can we get this closed?

GraphQL does have standard attributes on requests and responses and that is found in the specification found here:

Request: http://spec.graphql.org/October2021/#sec-Validating-Requests

Response: http://spec.graphql.org/October2021/#sec-Response-Format

So we can get this implemented in the instrumentation.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[Sytten]

Not stale

[dyladan]

Who would be the one to start the initiative to add GraphQL conventions to the spec?
As GraphQL is gaining popularity it should be a priority to have uniform telemetry data.

@jord1e Sorry for the late response. Anyone can submit a change to the opentelemetry spec. Following the steps in spec/CONTRIBUTING.md#Proposing a change, the first step would be to create an issue. Once the issue is accepted by the spec maintainers, then a PR can be drafted. In this case there is already an open issue and a PR which already has significant reviews.

@jscherer92 I'm sorry I wasn't clear I was referring to the OTel semantic convention specification. It looks like the process is already started with an issue and a PR already opened. The PR also has significant reviews so I expect it will merge soon.

[github-actions]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue was closed because it has been stale for 14 days with no activity.

[Sytten]

Auto close bots are a bad practice