chore(graphql): update span name to show the resolver name

[diogotorres97]

BEGIN_COMMIT_OVERRIDE
feat!(graphql): update span name to show the resolver name
END_COMMIT_OVERRIDE

Which problem is this PR solving?

Fixes #1791

Short description of the changes

This PR updates (opentelemetry-instrumentation-graphql) the resolvers' span name.
It will show something like: graphql-resolve hello instead of graphql-resolve. This is very useful to easily understand at a top level which field is being resolved.

E.g.,

query ExampleQuery {
  me {
    name 
    }
}

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: diogotorres97 / name: Diogo Torres (a92bfc4)
• ✅ login: dyladan / name: Daniel Dyla (c5c1e99)

[satazor]

Not sure who to ping here for visibility, cc @blumamir

[codecov]

Codecov Report

Merging #1796 (c5c1e99) into main (eee50d8) will decrease coverage by 0.43%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1796      +/-   ##
==========================================
- Coverage   91.42%   91.00%   -0.43%     
==========================================
  Files         143      133      -10     
  Lines        7303     6580     -723     
  Branches     1461     1311     -150     
==========================================
- Hits         6677     5988     -689     
+ Misses        626      592      -34     

Files	Coverage Δ
...opentelemetry-instrumentation-graphql/src/utils.ts	93.17% <ø> (ø)

... and 10 files with indirect coverage changes

[diogotorres97]

Rebased and fixed linter issues

[dyladan]

Looks like there's no code owner for these files. @open-telemetry/javascript-approvers please take a look if you have time

[Flarna]

To my understanding this doesn't fit the graphql semantic conventions.

I guess first step here should be a PR on the semantic-conventions repo to get consensus if this is wanted or not.

[diogotorres97]

To my understanding this doesn't fit the graphql semantic conventions.

I guess first step here should be a PR on the semantic-conventions repo to get consensus if this is wanted or not.

@Flarna I didn't understand why it doesn't fit, here we are updating the resolver part (inside of a query or mutation). That link refers to graphql.operation.type: query, mutation and subscription. But this is something more internal and not a graphql operation.

[dyladan]

It looks like there is no resolver in the semconv. It might be worth opening a semconv issue or PR to cover this area to ensure all languages are handling it the same way. Do we know if other languages have instrumentations for graphql and how they're handling this?

[Flarna]

Ah ok sorry. Seems I lack graphql knowledge.
As long as the names fit to the general span requirements it should be fine I think.

But I agree with @dyladan that a spec improvement might make sense if this is something other languages have also to keep consistency.

[dyladan]

As long as the names fit to the general span requirements it should be fine I think.

I agree. I'm not a graphql expert but I think this fits the requirement to be limited cardinality and should be fine. In any case it can't be higher cardinality than the attribute which should also be limited.

[jcarres-mdsol]

IMHO it does not fit the requirement of limited cardinality.
This graph is showing the cardinality of the operation name, the orange is our graphql in develop where we updated this library

Moving this to production would make this much worse. I think any organization with a lot going on with graphql will suffer from this.

[satazor]

@jcarres-mdsol I have to disagree. The document you linked lists examples of good and bad values in regards to cardinality. With this PR, the span name is no longer a generic name, increasing cardinality but it doesn’t include any dynamic identifier, such as a user id, which would violate the rules set in the document. I would argue that having resolve like it was before would actually violate the rules because it’s too generic (too generic is listed as a bad value).

Now span names in graphql resolvers are identifiable and we can make aggregations on which ones are slower etc, much like you have different span names for different functions etc.

If this raises concerns about performance or other issues, perhaps this could be under an option.

[jcarres-mdsol]

We have solved this with a regex in the collector.

The original operation names are something like x.y.z.a.b , it is true it is not infinite like if they were UUIDs or something but still 10s/100s of 1000s. So we have regex this to x.y this is enough in our case to identify it enough, also the whole path is keep in an attribute anyways.