Skip to content

fix: Enable union type support in @opentelemetry/instrumentation-graphql (#1506) #2923

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

Merged
david-luna merged 7 commits into from
Sep 22, 2025
Merged

fix: Enable union type support in @opentelemetry/instrumentation-graphql (#1506) #2923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9f087c7
fix union types
nitrofski Jun 3, 2025
b174583
Merge remote-tracking branch 'upstream/main' into dev/fortinv/fix-uni…
nitrofski Jul 9, 2025
8a08afd
lint
nitrofski Jul 10, 2025
c7f51ad
Merge branch 'main' into dev/fortinv/fix-union-types
nitrofski Jul 10, 2025
2da8f10
Merge branch 'main' into dev/fortinv/fix-union-types
nitrofski Aug 4, 2025
0c5ef73
Merge branch 'main' into dev/fortinv/fix-union-types
david-luna Sep 12, 2025
115cec5
Merge branch 'main' into dev/fortinv/fix-union-types
david-luna Sep 22, 2025
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
47 changes: 37 additions & 10 deletions packages/instrumentation-graphql/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,11 +305,7 @@ export function wrapFields(
tracer: api.Tracer,
getConfig: () => GraphQLInstrumentationParsedConfig
): void {
if (
!type ||
typeof type.getFields !== 'function' ||
Copy link
Contributor

@david-luna david-luna Sep 15, 2025

Choose a reason for hiding this comment

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

I'm not familiar enough with this library. Are we certain that all possible values of type have a function named getFields?

Copy link
Contributor Author

@nitrofski nitrofski Sep 15, 2025

Choose a reason for hiding this comment

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

It is part of graphqlTypes.GraphQLObjectType. As long as we maintain type safety and that the type definitions are accurate (to my testing, they were), we should be safe.

The previous implementation used an any type before recursing, so they lost type safety and ended up passing non-compliant object. It had to re-assess the object here.

Copy link
Contributor

@david-luna david-luna Sep 15, 2025

Choose a reason for hiding this comment

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

okay then

type[OTEL_PATCHED_SYMBOL]
) {
if (!type || type[OTEL_PATCHED_SYMBOL]) {
return;
}
const fields = type.getFields();
Expand All @@ -328,16 +324,47 @@ export function wrapFields(
}

if (field.type) {
let unwrappedType: any = field.type;

while (unwrappedType.ofType) {
unwrappedType = unwrappedType.ofType;
const unwrappedTypes = unwrapType(field.type);
for (const unwrappedType of unwrappedTypes) {
wrapFields(unwrappedType, tracer, getConfig);
}
wrapFields(unwrappedType, tracer, getConfig);
}
});
}

function unwrapType(
type: graphqlTypes.GraphQLOutputType
): readonly graphqlTypes.GraphQLObjectType[] {
// unwrap wrapping types (non-nullable and list types)
if ('ofType' in type) {
return unwrapType(type.ofType);
}

// unwrap union types
if (isGraphQLUnionType(type)) {
return type.getTypes();
Copy link
Contributor

@david-luna david-luna Sep 15, 2025

Choose a reason for hiding this comment

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

Here I have a similar question. Are we certain that getTypes() returns an array of GraphQLObjectType. Could be possible to have nested unions?

Copy link
Contributor Author

@nitrofski nitrofski Sep 15, 2025

Choose a reason for hiding this comment

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

Fair concern. Doing a quick search through the graphql type definitions, GraphQLUnionType is the only type with a getTypes() function, and it always returns an array of GraphQLObjectType. That all being said, is it enough for us or do we want to be extra safe and also validate what the function returns at runtime?

Copy link
Contributor Author

@nitrofski nitrofski Sep 15, 2025

Choose a reason for hiding this comment

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

Could be possible to have nested unions?

Doing a bit of research...

From the Apollo Graphql Union and Interfaces schema help page

All of a union's included types must be object types (not scalars, input types, etc.). Included types do not need to share any fields.

It sounds like unions strictly require their components to be objects and that nested union are not allowed. Though, to dispel any doubts, I tried launching my graphql server with a dummy union type that uses other union types:

Union type MyTestUnion can only include Object types, it cannot include <other union type>.

Copy link
Contributor

@david-luna david-luna Sep 15, 2025

Choose a reason for hiding this comment

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

thanks for your thorough explanation @nitrofski :)

}

// return object types
if (isGraphQLObjectType(type)) {
return [type];
}

return [];
}

function isGraphQLUnionType(
type: graphqlTypes.GraphQLType
): type is graphqlTypes.GraphQLUnionType {
return 'getTypes' in type && typeof type.getTypes === 'function';
}

function isGraphQLObjectType(
type: graphqlTypes.GraphQLType
): type is graphqlTypes.GraphQLObjectType {
return 'getFields' in type && typeof type.getFields === 'function';
}

const handleResolveSpanError = (
resolveSpan: api.Span,
err: any,
Expand Down
121 changes: 121 additions & 0 deletions packages/instrumentation-graphql/test/graphql.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,21 @@ const sourceFindUsingVariable = `
}
`;

const sourceSearch = `
query Search ($name: String!) {
search(name: $name) {
... on Book {
__typename
name
}
... on EBook {
__typename
name
}
}
}
`;

const badQuery = `
query foo bar
`;
Expand Down Expand Up @@ -244,6 +259,7 @@ describe('graphql', () => {
assert.ok(times[RESOLVE].end <= times[EXECUTE].end);
});
});

describe('AND source is query with param', () => {
let spans: ReadableSpan[];

Expand Down Expand Up @@ -338,6 +354,7 @@ describe('graphql', () => {
);
});
});

describe('AND source is query with param and variables', () => {
let spans: ReadableSpan[];

Expand Down Expand Up @@ -442,6 +459,110 @@ describe('graphql', () => {
);
});
});

describe('AND source is query to get a list of union type', () => {
let spans: ReadableSpan[];
beforeEach(async () => {
create({});
await graphql({
schema,
source: sourceSearch,
variableValues: { name: 'first' },
});
spans = exporter.getFinishedSpans();
});

afterEach(() => {
exporter.reset();
graphQLInstrumentation.disable();
spans = [];
});

it('should have 6 spans', () => {
assert.deepStrictEqual(spans.length, 6);
});

it('should instrument parse', () => {
const parseSpan = spans[0];
assert.deepStrictEqual(
parseSpan.attributes[AttributeNames.SOURCE],
sourceSearch
);
assert.deepStrictEqual(parseSpan.name, SpanNames.PARSE);
});

it('should instrument validate', () => {
const validateSpan = spans[1];

assert.deepStrictEqual(validateSpan.name, SpanNames.VALIDATE);
assert.deepStrictEqual(
validateSpan.parentSpanContext?.spanId,
undefined
);
});

it('should instrument execute', () => {
const executeSpan = spans[5];

assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.SOURCE],
sourceSearch
);
assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.OPERATION_TYPE],
'query'
);
assert.deepStrictEqual(
executeSpan.attributes[AttributeNames.OPERATION_NAME],
'Search'
);
assert.deepStrictEqual(executeSpan.name, 'query Search');
assert.deepStrictEqual(
executeSpan.parentSpanContext?.spanId,
undefined
);
});

it('should instrument resolvers', () => {
const [, , resolveParentSpan, span1, span2, executeSpan] = spans;

assertResolveSpan(
resolveParentSpan,
'search',
'search',
'[SearchResult]',
'search(name: $name) {\n' +
' ... on Book {\n' +
' __typename\n' +
' name\n' +
' }\n' +
' ... on EBook {\n' +
' __typename\n' +
' name\n' +
' }\n' +
' }',
executeSpan.spanContext().spanId
);

const parentId = resolveParentSpan.spanContext().spanId;
assertResolveSpan(
span1,
'name',
'search.0.name',
'String',
'name',
parentId
);
assertResolveSpan(
span2,
'name',
'search.1.name',
'String',
'name',
parentId
);
});
});
});

describe('when depth is set to 0', () => {
Expand Down
Loading
Loading