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

fix: update GraphQL span name to follow semantic conventions #966

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: update GraphQL span name to follow semantic conventions #966

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
370af56
fix: update GraphQL span name to follow semantic conventions
karmingc May 3, 2024
52255d5
Merge branch 'open-telemetry:main' into kc/fix/graphql-sem
karmingc May 10, 2024
691c441
Merge branch 'main' into kc/fix/graphql-sem
kaylareopelle May 14, 2024
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
10 changes: 7 additions & 3 deletions instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,15 @@ def analyze_query(query:, &block)

def execute_query(query:, &block)
attributes = {}
attributes['graphql.operation.name'] = query.selected_operation_name if query.selected_operation_name
attributes['graphql.operation.type'] = query.selected_operation.operation_type
operation_type = query.selected_operation.operation_type
operation_name = query.selected_operation_name

attributes['graphql.operation.name'] = operation_name if operation_name
attributes['graphql.operation.type'] = operation_type
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a MUST-level part of the spec requiring operation.type to be one of query, mutation or subscription. We weren't checking it previously. I'm not certain we need to validate it now.

Will the gem only send those operation types? What are your thoughts @karmingc?

attributes['graphql.document'] = query.query_string

tracer.in_span('graphql.execute_query', attributes: attributes, &block)
span_name = operation_name ? "#{operation_type} #{operation_name}" : operation_type
Copy link
Contributor

@robertlaurin robertlaurin May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't make too much sense at the spec level. OpenTelemetry consistently discourages high cardinality span names, and this by definition is unbounded cardinality.

Edit to add context. GraphQL operation name is just a free form label supplied by the client caller. There's absolutely no constraints, if we don't put the raw path in an http server span name, I'm not at all convinced we should all operation name here.

Fwiw query, mutation, subscription seems reasonable on its own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@robertlaurin Did you open an issue in the semantic-conventions repo about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It does not seem like that came up in the PR reviews:

open-telemetry/opentelemetry-specification#2456
open-telemetry/opentelemetry-specification#1670

Copy link
Contributor

Choose a reason for hiding this comment

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

@robertlaurin - Could you open an issue in the semantic-conventions repo about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@karmingc, I apologize for how long the cycles on this PR have been. I brought this concern to the Semantic Conventions repo.

Issue: open-telemetry/semantic-conventions#1361
PR: open-telemetry/semantic-conventions#1389

If you have any feedback on the proposed span name, let's discuss it in the PR. (cc @robertlaurin @arielvalentin)

tracer.in_span(span_name, attributes: attributes, &block)
end

def execute_query_lazy(query:, multiplex:, &block)
Expand Down
14 changes: 13 additions & 1 deletion instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class GraphQLTracer < ::GraphQL::Tracing::PlatformTracing
def platform_trace(platform_key, key, data)
return yield if platform_key.nil?

tracer.in_span(platform_key, attributes: attributes_for(key, data)) do |span|
tracer.in_span(span_name(platform_key, data), attributes: attributes_for(key, data)) do |span|
yield.tap do |response|
next unless key == 'validate'

Expand Down Expand Up @@ -143,6 +143,18 @@ def attr_cache
cache_h.compare_by_identity
cache_h
end

def span_name(key, data)
case key
when @platform_keys['execute_query']
operation_type = data[:query].selected_operation.operation_type
operation_name = data[:query].selected_operation_name

operation_name ? "#{operation_type} #{operation_name}" : operation_type
else
key
end
end
end
end
end
Expand Down
6 changes: 3 additions & 3 deletions instrumentation/graphql/test/instrumentation/graphql/tracers/graphql_trace_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
'graphql.validate',
'graphql.analyze_query',
'graphql.analyze_multiplex',
'graphql.execute_query',
'query',
'graphql.execute_query_lazy',
'graphql.execute_multiplex'
]
Expand All @@ -76,7 +76,7 @@

SomeGraphQLAppSchema.execute('query SimpleQuery{ simpleField }')

span = spans.find { |s| s.name == 'graphql.execute_query' }
span = spans.find { |s| s.name == 'query SimpleQuery' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
end
Expand All @@ -89,7 +89,7 @@

SomeGraphQLAppSchema.execute('{ simpleField }')

span = spans.find { |s| s.name == 'graphql.execute_query' }
span = spans.find { |s| s.name == 'query' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
end
Expand Down
6 changes: 3 additions & 3 deletions instrumentation/graphql/test/instrumentation/graphql/tracers/graphql_tracer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
'graphql.validate',
'graphql.analyze_query',
'graphql.analyze_multiplex',
'graphql.execute_query',
'query',
'graphql.execute_query_lazy',
'graphql.execute_multiplex'
]
Expand All @@ -75,7 +75,7 @@

SomeGraphQLAppSchema.execute('query SimpleQuery{ simpleField }')

span = spans.find { |s| s.name == 'graphql.execute_query' }
span = spans.find { |s| s.name == 'query SimpleQuery' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
end
Expand All @@ -89,7 +89,7 @@

SomeGraphQLAppSchema.execute('{ simpleField }')

span = spans.find { |s| s.name == 'graphql.execute_query' }
span = spans.find { |s| s.name == 'query' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
end
Expand Down