search

open-telemetry / semantic-conventions

Defines standards for generating consistent, accessible telemetry across a variety of domains
Apache License 2.0
270 stars 174 forks source link

Concerns about high cardinality in GraphQL span names #1361

Closed kaylareopelle closed 3 weeks ago

kaylareopelle commented 2 months ago

Area(s)

area:graphql

Is your change request related to a problem? Please describe.

The Ruby SIG has some concerns about the cardinality of span names for the current GraphQL semantic convention.

@karmingc opened a PR on the opentelemetry-ruby-contrib repo to update the GraphQL span names.

This would update Ruby's existing name, graphql.execute_query, to the current semantic convention's format, <graphql.operation.type> <graphql.operation.name>.

During review, @robertlaurin raised a concern about unbounded cardinality for the span names if operation name and operation type are included:

...OpenTelemetry consistently discourages high cardinality span names, and this by definition is unbounded cardinality.

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. (source)

The PR is currently blocked until we can come to a resolution about the appropriate span name. 


Describe the solution you'd like

<graphql.operation.type> seems sufficient, since the <graphql.operation.name> is available in span attributes.

Describe alternatives you've considered

The GraphQL semantic convention could also be updated to more closely match the DB span name, by adding a SHOULD instead of a MUST for the name and/or add the low_cardinality caveat about db.operation.name.

The span name SHOULD be {db.operation.name} {target} if there is a (low-cardinality) {db.operation.name} available (see below for the exact definition of the {target} placeholder). [source]

Additional context

This may be similar to https://github.com/open-telemetry/semantic-conventions/issues/182, but since the problem focuses specifically on the span name, I thought it warranted a new issue.

joaopgrassi commented 2 months ago

@kaylareopelle since you folks have good ideas on how to move forward, are you opening a PR to change it?

kaylareopelle commented 2 months ago

Hi @joaopgrassi! I can open a PR to change it! This is my first time interacting with the semantic conventions repo, so I wasn't exactly sure about the process.

joaopgrassi commented 2 months ago

Perfect, thank you! I will assign this to you then. Feel free to reach out if you need help with anything.

kaylareopelle commented 2 months ago

Hi @joaopgrassi! I've opened a PR for this change: https://github.com/open-telemetry/semantic-conventions/pull/1389