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 and
xuan-cao-swi
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
There was a problem hiding this comment.
@robertlaurin - Could you open an issue in the semantic-conventions repo about this?
There was a problem hiding this comment.
@karmingc, I apologize for how long the cycles on this PR have been. I brought this concern to the Semantic Conventions repo.
Issue: open-telemetry/semantic-conventions#1361
PR: open-telemetry/semantic-conventions#1389
If you have any feedback on the proposed span name, let's discuss it in the PR. (cc @robertlaurin @arielvalentin)
| 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?
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
arielvalentin
added
keep
|
Hi @karmingc! Whew, this has been a journey! I appreciate your patience. We brought these questions to the semantic conventions repo. After some discussion, the decision was to change the span name to be With this in mind, are you interested in updating this PR to fit with the new semantic convention? |
Hi, I haven't had the chance to comment/reply for a while given that I am no longer involved on Ruby-related applications at work. However, I'm still open to fixing these! I'll try to update in the upcoming weeks if possible. If somebody needs this to be prioritized, feel free to pick it up as well. |
The semantic convention for GraphQL spans is specified here: https://opentelemetry.io/docs/specs/semconv/graphql/graphql-spans/. This will also resolve #560.