feat(instrumentation-graphql): change execute and parse span names and attributes #747

mentos1386 · 2021-11-16T22:37:09Z

Which problem is this PR solving?

Fixes partially Graphql should add operation name and hash in span #705 (without the hash request)
Fixes Add Resolver field name to graphql instrumentation span name #534

Short description of the changes

Execute span names changed from graphq.execute to graphql.execute.query, where query represents an operation type.
Prepare span names changed from graphql.prepare to graphql.prepare.user wher user is field name.
Renamed graphql.operation.name to graphql.operation.type which represents mutation,query or subscription.
Added graphql.operation.name which representes Query1 as operation name.

mentos1386 · 2021-11-16T22:42:19Z

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

      }
-      operationName = OPERATION_NOT_SUPPORTED.replace(


I have removed this, as i don't believe it's needed to be presented in such a way. Maybe we should rather add an attribute that would represent that the operation is "not supported"?

why do you think so ?
currently it will help to identify the operation name that is not supported, after yr change it will not be possible, so I see here a regression instead of improvement.

That is true, but making operationName to be Operation ${operationName} not supported breaks what the "opeationName" field is supposed to be.

I would stick with "opertationName" to represent the name so the value is more predictable. Especially if you want to use it with some automation, where with current implementation you would have to parse the string to get out the actual operation name.

Maybe adding operationUnsupported: true or something in this sense, to indicate that operation is not supported?

plugins/node/opentelemetry-instrumentation-graphql/src/enum.ts

dyladan · 2021-11-16T22:50:08Z

plugins/node/opentelemetry-instrumentation-graphql/src/enums/AttributeNames.ts

@@ -19,7 +19,8 @@ export enum AttributeNames {
  FIELD_NAME = 'graphql.field.name',
  FIELD_PATH = 'graphql.field.path',
  FIELD_TYPE = 'graphql.field.type',
-  OPERATION = 'graphql.operation.name',
+  OPERATION_TYPE = 'graphql.operation.type',


Is this specified?

I don't understand what you mean @dyladan.

@mentos1386 I suspect @dyladan is asking whether there are OpenTelemetry semantic conventions for GraphQL trace data. (see here for what that means if you're unsure: #705 (comment))

If so, the answer is "not yet"; the specification effort is underway here: open-telemetry/opentelemetry-specification#1670

Yes @spencerwilson is correct that's exactly what I meant. In general we hesitate to add/change attributes without semantic conventions, but in this case it seems like the spec is moving quite slowly.

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

codecov · 2021-11-17T19:06:03Z

Codecov Report

Merging #747 (39be853) into main (0463f27) will decrease coverage by 1.92%.
The diff coverage is 97.91%.

❗ Current head 39be853 differs from pull request most recent head f5bd5d9. Consider uploading reports for the commit f5bd5d9 to get more accurate results

@@            Coverage Diff             @@
##             main     #747      +/-   ##
==========================================
- Coverage   96.87%   94.94%   -1.93%     
==========================================
  Files          11       22      +11     
  Lines         640     1603     +963     
  Branches      126      226     +100     
==========================================
+ Hits          620     1522     +902     
- Misses         20       81      +61

Impacted Files	Coverage Δ
...opentelemetry-instrumentation-graphql/src/types.ts	`100.00% <ø> (ø)`
...entelemetry-instrumentation-graphql/test/helper.ts	`95.45% <90.90%> (ø)`
...nstrumentation-graphql/src/enums/AttributeNames.ts	`100.00% <100.00%> (ø)`
...try-instrumentation-graphql/src/instrumentation.ts	`92.89% <100.00%> (ø)`
...opentelemetry-instrumentation-graphql/src/utils.ts	`93.65% <100.00%> (ø)`
...metry-instrumentation-graphql/test/graphql.test.ts	`100.00% <100.00%> (ø)`
...ages/auto-instrumentations-node/test/utils.test.ts	`96.87% <0.00%> (ø)`
... and 7 more

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

obecny · 2021-11-18T16:52:31Z

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

      }
-      operationName = OPERATION_NOT_SUPPORTED.replace(


why do you think so ?
currently it will help to identify the operation name that is not supported, after yr change it will not be possible, so I see here a regression instead of improvement.

plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts

…d attributes

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

spencerwilson · 2021-12-17T09:09:41Z

plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts

+export const createResolveSpanName = (fieldName: string): string => {
+  return `${SpanNames.RESOLVE}.${fieldName}`;
+};
+
+export const createExecuteSpanName = (
+  operationType: string | undefined
+): string => {
+  if (operationType) {
+    return `${SpanNames.EXECUTE}.${operationType}`;
+  }
+  return `${SpanNames.EXECUTE}`;
+};


What's your motivation in adding low-cardinality (field name) and unbounded-cardinality (operation name) data to these spans' names? I'm concerned that this change will make queries on these span names harder, since these names would now have two jobs: convey the span "type" + convey another aspect of the span. E.g., it's harder to refer to all the "resolver" spans. The added data is present in other attributes; is that not sufficient? Each attribute describing a single characteristic of the span? If one wished to do a query akin to how "GROUP BY name" would be under these changes, one would do "GROUP BY name, field_name".

Apologies; just saw the linked issue. Commented there.

I believe it's adding more visibility (without looking at attributes) at what part of "graphql resolving" does the span belongs to. Same as for HTTP there isn't just "http.resolving" but it includes method (GET, POST etc.) and also the "route" such as /api/users/{user_id}.

https://opentelemetry.io/docs/reference/specification/trace/semantic_conventions/http/#name

mentos1386 · 2022-01-03T08:26:12Z

Any update on this @obecny? Should we rather wait for a spec to be written first (@spencerwilson)?

obecny · 2022-01-04T08:49:26Z

Any update on this @obecny? Should we rather wait for a spec to be written first (@spencerwilson)?

Mine main concern is that this is still not spec'ed and it might be tricky later to make necessary changes. I don't really have strong opinion here, will simply defer this to the @open-telemetry/javascript-maintainers to decide about this PR

github-actions · 2022-03-07T06:35:26Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions · 2022-03-21T06:35:54Z

This PR was closed because it has been stale for 14 days with no activity.

mentos1386 requested a review from a team November 16, 2021 22:37

github-actions bot assigned obecny Nov 16, 2021

github-actions bot requested a review from obecny November 16, 2021 22:40

mentos1386 commented Nov 16, 2021

View reviewed changes

dyladan reviewed Nov 16, 2021

View reviewed changes

mentos1386 requested a review from dyladan November 17, 2021 19:04

obecny reviewed Nov 18, 2021

View reviewed changes

mentos1386 and others added 4 commits November 24, 2021 22:29

feat(instrumentation-graphql): change execute and parse span names an…

be7a970

…d attributes

chore(instrumentation-graphql): remove operation name from execute span

f78ec52

chore(instrumentation-graphql): use es6 syntax and strict comparison

ccc0354

Co-authored-by: Bartlomiej Obecny <bobecny@gmail.com>

chore(instrumentation-graphql): use es6 syntaxt and strict comparison

f5bd5d9

mentos1386 requested a review from obecny November 24, 2021 21:30

spencerwilson reviewed Dec 17, 2021

View reviewed changes

github-actions bot added the stale label Mar 7, 2022

github-actions bot closed this Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(instrumentation-graphql): change execute and parse span names and attributes #747

feat(instrumentation-graphql): change execute and parse span names and attributes #747

mentos1386 commented Nov 16, 2021 •

edited

Loading

mentos1386 Nov 16, 2021 •

edited

Loading

obecny Nov 18, 2021

mentos1386 Nov 24, 2021

dyladan Nov 16, 2021

mentos1386 Nov 17, 2021

spencerwilson Dec 19, 2021 •

edited

Loading

dyladan Jan 3, 2022

codecov bot commented Nov 17, 2021 •

edited

Loading

obecny Nov 18, 2021

spencerwilson Dec 17, 2021 •

edited

Loading

spencerwilson Dec 17, 2021

mentos1386 Dec 17, 2021

mentos1386 commented Jan 3, 2022 •

edited

Loading

obecny commented Jan 4, 2022

github-actions bot commented Mar 7, 2022

github-actions bot commented Mar 21, 2022

feat(instrumentation-graphql): change execute and parse span names and attributes #747

feat(instrumentation-graphql): change execute and parse span names and attributes #747

Conversation

mentos1386 commented Nov 16, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

mentos1386 Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerwilson Dec 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

spencerwilson Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mentos1386 commented Jan 3, 2022 • edited Loading

obecny commented Jan 4, 2022

github-actions bot commented Mar 7, 2022

github-actions bot commented Mar 21, 2022

mentos1386 commented Nov 16, 2021 •

edited

Loading

mentos1386 Nov 16, 2021 •

edited

Loading

spencerwilson Dec 19, 2021 •

edited

Loading

codecov bot commented Nov 17, 2021 •

edited

Loading

spencerwilson Dec 17, 2021 •

edited

Loading

mentos1386 commented Jan 3, 2022 •

edited

Loading