Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be7a970
feat(instrumentation-graphql): change execute and parse span names an…
mentos1386 Nov 16, 2021
f78ec52
chore(instrumentation-graphql): remove operation name from execute span
mentos1386 Nov 17, 2021
ccc0354
chore(instrumentation-graphql): use es6 syntax and strict comparison
mentos1386 Nov 24, 2021
f5bd5d9
chore(instrumentation-graphql): use es6 syntaxt and strict comparison
mentos1386 Nov 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugins/node/opentelemetry-instrumentation-graphql/src/enums/AttributeNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean @dyladan.

Copy link

@spencerwilson spencerwilson Dec 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

OPERATION_NAME = 'graphql.operation.name',
VARIABLES = 'graphql.variables.',
ERROR_VALIDATION_NAME = 'graphql.validation.error',
}
53 changes: 35 additions & 18 deletions plugins/node/opentelemetry-instrumentation-graphql/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ import {
GraphQLInstrumentationParsedConfig,
OtelExecutionArgs,
ObjectWithGraphQLData,
OPERATION_NOT_SUPPORTED,
Maybe,
} from './types';
import {
addInputVariableAttributes,
addSpanSource,
createExecuteSpanName,
endSpan,
getOperation,
wrapFieldResolver,
Expand Down Expand Up @@ -390,29 +390,46 @@ export class GraphQLInstrumentation extends InstrumentationBase {
});
}

private getOperationType(
operation: graphqlTypes.DefinitionNode | undefined
): string | undefined {
if (operation?.kind === 'OperationDefinition') {
return operation.operation;
}

return undefined;
}

private getOperationName(
operation: graphqlTypes.DefinitionNode | undefined,
processedArgs: graphqlTypes.ExecutionArgs
): string | undefined {
if (operation?.kind === 'OperationDefinition') {
if (operation.name) {
return operation.name.value;
}
}

return processedArgs.operationName ?? undefined;
}

private _createExecuteSpan(
operation: graphqlTypes.DefinitionNode | undefined,
processedArgs: graphqlTypes.ExecutionArgs
): api.Span {
const config = this._getConfig();

const span = this.tracer.startSpan(SpanNames.EXECUTE, {});
if (operation) {
const name = (operation as graphqlTypes.OperationDefinitionNode)
.operation;
if (name) {
span.setAttribute(AttributeNames.OPERATION, name);
}
} else {
let operationName = ' ';
if (processedArgs.operationName) {
operationName = ` "${processedArgs.operationName}" `;
}
operationName = OPERATION_NOT_SUPPORTED.replace(
Copy link
Contributor Author

@mentos1386 mentos1386 Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

'$operationName$',
operationName
);
span.setAttribute(AttributeNames.OPERATION, operationName);
const operationType = this.getOperationType(operation);
const operationName = this.getOperationName(operation, processedArgs);

const span = this.tracer.startSpan(createExecuteSpanName(operationType));

if (operationType) {
span.setAttribute(AttributeNames.OPERATION_TYPE, operationType);
}

if (operationName) {
span.setAttribute(AttributeNames.OPERATION_NAME, operationName);
}

if (processedArgs.document?.loc) {
Expand Down
3 changes: 0 additions & 3 deletions plugins/node/opentelemetry-instrumentation-graphql/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,6 @@ import {
import { GraphQLSchema } from 'graphql/type/schema';
import { OTEL_GRAPHQL_DATA_SYMBOL, OTEL_PATCHED_SYMBOL } from './symbols';

export const OPERATION_NOT_SUPPORTED =
'Operation$operationName$not' + ' supported';

export interface GraphQLInstrumentationExecutionResponseHook {
(span: api.Span, data: graphqlTypes.ExecutionResult): void;
}
Expand Down
15 changes: 14 additions & 1 deletion plugins/node/opentelemetry-instrumentation-graphql/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ function createResolverSpan(
};

const span = tracer.startSpan(
SpanNames.RESOLVE,
createResolveSpanName(info.fieldName),
{
attributes,
},
Expand Down Expand Up @@ -433,3 +433,16 @@ async function safeExecuteInTheMiddleAsync<T>(
return result as T;
}
}

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}`;
};
Comment on lines +437 to +448
Copy link

@spencerwilson spencerwilson Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies; just saw the linked issue. Commented there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading