Normalize GraphQL span names for easier aggregation analysis

[ravangen]

For the GraphQL integration, this:

• introduces new attributes to existing platform spans
• seeks to normalize platform span names for easier identification and future aggregation

New Attributes

Builds on open-telemetry/opentelemetry-specification#2456 and work done in opentelemetry-js-contrib

There are some differences between the GraphQL implementations in different languages that may make it harder to have globally consistent names without working with the library authors. For example, graphql-ruby separates out reporting of lazy (delayed, possibly async) work from its sync work, resulting in multiple spans for one graphql-js "resolve" concept.

Field resolve/execute

• graphql.field.parent: Field's parent type. For interfaces, this is the concrete type's name, not interface name.
• graphql.field.name: Field's name, matches opentelemetry-js
• graphql.lazy: Is this the lazy part of the resolver. There will always at least an inline/sync portion (e.g. false), and will sometimes have lazy work to be subsequently done at a later time. No opentelemetry-js equivalent as they use promises.
• graphql.field.path: Did not add at this time, but the information is available via graphql-ruby if we want to report it. There is a opentelemetry-js equivalent. Thoughts?

Type authorization

• graphql.type.name: Type name having its authorization check run
• graphql.lazy: Is this the lazy part of the authorization.

Type resolution

When a field’s return type is an interface, GraphQL has to figure out what specific object type to use for the return value.

• graphql.type.name: Interface name having its object type determined
• graphql.lazy: Is this the lazy part of the type resolution.

Span Name Normalization

In talking with @robertlaurin about how we can use this data for analysis across many requests, we found we wanted to have a consistent span name to easily identify specify types of spans. So I moved the type/field names to attributes.

This change was made in a backwards compatible manner via legacy_platform_span_names config. To keep the original behaviour, set true. I figure if/when this gem moves to v1, we can remove this config option.

[robertlaurin]

graphql.field.path: Did not add at this time, but the information is available via graphql-ruby if we want to report it. There is a opentelemetry-js equivalent. Thoughts?

Just to clarify I understand what his value would represent.

Using one of the test examples of { vehicle { model } }, would the path for model field be vehicle.model?

[ravangen]

graphql.field.path: Did not add at this time, but the information is available via graphql-ruby if we want to report it. There is a opentelemetry-js equivalent. Thoughts?

Just to clarify I understand what his value would represent.

Using one of the test examples of { vehicle { model } }, would the path for model field be vehicle.model?

Yes. It would be a path with array of elements joined by .

A field with a list of elements would use the index of the element, vehicles.1.model

[ahayworth]

I have no major concerns here - I should note that I do not really know much about graphql in general so the utility of this is not something I can comment on. However, I think I see the value of normalizing these names for analysis.

[ahayworth]

I saw in the PR message that we would consider flipping this from false -> true if instrumentation ever hits 1.0 (we're not in a rush).

Why wait? It's not stable now; and if we think this is a useful enough change to flip at 1.0 why not now?

This is not a blocking comment, I'm just curious what you think here.

[ravangen]

Wasn't sure on this gem's policy on breaking changes. We can change the default to false and keep the setting to have the option of using the old behaviour, no strong opinions.

[ahayworth]

I don't think we have a super strong policy for things that are pre-1.0 honestly.

[robertlaurin]

@ravangen feel free to just add rubocop disable comment for this.

lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb:85:11: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for attributes_for is too high. [10/7]
          def attributes_for(key, data) ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[arielvalentin]

👋🏼 @rmosolgo if you have any insights you would be interested in sharing.