fix: update GraphQL span name to follow semantic conventions #966
Conversation
karmingc
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
karmingc
force-pushed
the
kc/fix/graphql-sem
branch
4 times, most recently
from
a8356e5
to
fc34086
Compare
The semantic convention for GraphQL spans is specified here: https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/. This will also resolve open-telemetry#560.
karmingc
force-pushed
the
kc/fix/graphql-sem
branch
from
fc34086
to
370af56
Compare
|
Where the names always wrong? I will have to take a look at the spec but are there any expectations around backward compatibility like there is with HTTP or Messaging semantics? cc: @rmosolgo |
@arielvalentin, all the GraphQL semantic conventions for spans are experimental. Breaking changes are allowed. |
| attributes['graphql.document'] = query.query_string | ||
|
|
||
| tracer.in_span('graphql.execute_query', attributes: attributes, &block) | ||
| span_name = operation_name ? "#{operation_type} #{operation_name}" : operation_type |
There was a problem hiding this comment.
This doesn't make too much sense at the spec level. OpenTelemetry consistently discourages high cardinality span names, and this by definition is unbounded cardinality.
Edit to add context. GraphQL operation name is just a free form label supplied by the client caller. There's absolutely no constraints, if we don't put the raw path in an http server span name, I'm not at all convinced we should all operation name here.
Fwiw query, mutation, subscription seems reasonable on its own.
There was a problem hiding this comment.
@robertlaurin Did you open an issue in the semantic-conventions repo about this?
There was a problem hiding this comment.
It does not seem like that came up in the PR reviews:
open-telemetry/opentelemetry-specification#2456
open-telemetry/opentelemetry-specification#1670
| operation_name = query.selected_operation_name | ||
|
|
||
| attributes['graphql.operation.name'] = operation_name if operation_name | ||
| attributes['graphql.operation.type'] = operation_type |
There was a problem hiding this comment.
There's a MUST-level part of the spec requiring operation.type to be one of query, mutation or subscription. We weren't checking it previously. I'm not certain we need to validate it now.
Will the gem only send those operation types? What are your thoughts @karmingc?
The semantic convention for GraphQL spans is specified here: https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/. This will also resolve #560.