feat!: Cache GraphQL attributes #867
Conversation
elenatanasoiu
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle and
xuan-cao-swi
as code owners
elenatanasoiu
force-pushed
the
elena/reduce-objects-for-fun-and-profit
branch
from
cfc84f7 to
485f4b6
Compare
instrumentation/graphql/lib/opentelemetry/instrumentation/graphql/tracers/graphql_tracer.rb
Show resolved
Hide resolved
Some can't be nil though, like `operation.name` and `field.parent`
elenatanasoiu
changed the title
Since we now mark these as the operation name.
|
👋 Hi @rmosolgo, I would really appreciate your opinion here since this is backporting the work you did previously to cache attribute hashes in v2. Thanks! 🙇♀️ |
|
@rmosolgo we sure could use your help reviewing this PR. We are not sure how to trigger an We also want to make sure we are not doing anything that could get us in trouble with attribute caching based on the Derived Type names as opposed to using the field objects themselves. We just could not figure out how to make it work without making special keys bound to the Owner type as opposed to the Field type in the legacy tracer. Your guidance would be greatly appreciated!!! |
|
After adding ff6aa06 we can see the performance improvement is much smaller. allocated objects by gem
-----------------------------------
5820 graphql-1.12.24
5488 opentelemetry-sdk-1.4.0
2353 opentelemetry-api-1.2.4
- 786 graphql/lib
+ 433 graphql/lib (it was 32 before we introduced the new commit)
523 other
allocated memory by gem
-----------------------------------
707840 opentelemetry-sdk-1.4.0
684008 graphql-1.12.24
194472 opentelemetry-api-1.2.4
- 130896 graphql/lib
+ 38928 graphql/lib (it was 3616 before we introduced the new commit)
41176 otherWe added the commit to satisfy this first test which expects the For graphql < 2.0.6, We went with getting the parent name off of |
|
@elenatanasoiu based on the results of the pairing session today I am seeing the value of your perspective, i.e. performance is more important than precision of Users who are on older versions of the GraphQL gem and tracer should not expect to have the same precision as a newer version. Not being able to see the derived types may incentivize folks to upgrade the gem or move to the most recent version of the tracer. It also occurs to me that these attributes are disabled by default due to performance overhead so I imagine most users won't enable it by default as we have not ever received any bug reports related to this problem. All that being said I am interested in hearing @rmosolgo perspective on the topic as well as the rest of the @open-telemetry/ruby-contrib-approvers |
|
👋 This change looks good to me! It sounds like you discovered the difference in As far as Here's a little example to demonstrate: Trace#execute_field_lazy example
require "bundler/inline"
gemfile do
gem "graphql", "2.2.8"
end
class MySchema < GraphQL::Schema
class Box # IRL this would probably be a Promise, Concurrent::Future, etc
def initialize(&get_value)
@get_value = get_value
end
def value
@get_value.call
end
end
class Query < GraphQL::Schema::Object
field :int, Integer
def int
puts "calling #int"
Box.new {
puts "resolving Box"
5
}
end
end
module Trace
def execute_field(**)
puts "Trace#execute_field"
super
end
def execute_field_lazy(**)
puts "Trace#execute_field_lazy"
super
end
end
lazy_resolve Box, :value
query(Query)
trace_with(Trace)
end
pp MySchema.execute("{ int }").to_h
# Trace#execute_field
# calling #int
# Trace#execute_field_lazy
# resolving Box
# {"data"=>{"int"=>5}} |
|
Yes that's right. That is making it difficult for us to cache fields. Do you have any recommendations on changes we can make to be able to build up a performant cache using the owner name? |
|
In my experience, it ends up reporting different names for fields, but I think there's a case to be made either way:
Since adding this new tracing code and migrating GraphQL-Ruby's other tracers to use it, I haven't had any issues raised because of the change. The only two suggestions I have are:
I don't know of any options besides those 🙈 ! |
|
Thank you sir. @elenatanasoiu lets go the route of better perf. |
elenatanasoiu
force-pushed
the
elena/reduce-objects-for-fun-and-profit
branch
from
7503899 to
0aae8ce
Compare
We're reverting open-telemetry@ff6aa06 so we can maximize caching benefits.
elenatanasoiu
force-pushed
the
elena/reduce-objects-for-fun-and-profit
branch
from
0aae8ce to
c01c7e5
Compare
|
@arielvalentin Good stuff! 👍 @rmosolgo Thank you for the careful considerations! 🙇♀️ I've pushed the changes up based on your recommendations: choosing performance. I've very much looking forward to trying this out once it's released. I've also attempted to add another test for I think we can add these tests separately from this PR which is now ready for re-review! I've left some notes below. Tracing execute_field_lazy# test_helper.rb
class Box # IRL this would probably be a Promise, Concurrent::Future, etc
def initialize(&get_value)
@get_value = get_value
end
def value
@get_value.call
end
end
class BoxQuery < GraphQL::Schema::Object
field :int, Integer
def int
puts "calling #int"
Box.new {
puts "resolving Box"
5
}
end
end
class BoxGraphQLAppSchema < GraphQL::Schema
module Trace
def execute_field(**)
puts "Trace#execute_field"
super
end
def execute_field_lazy(**)
puts "Trace#execute_field_lazy"
super
end
end
lazy_resolve Box, :value
query ::BoxQuery
trace_with Trace
end# graphql_tracer_test.rb
it 'includes attributes using platform types - lazy' do
skip if uses_platform_interfaces?
expected_attributes = {
'graphql.field.parent' => 'Box', # type name, not interface
'graphql.field.name' => 'int',
'graphql.lazy' => false
}
BoxGraphQLAppSchema.execute('{ int }')
require 'pry'; binding.pry
span = spans.find { |s| s.name == 'graphql.execute_field_lazy' && s.attributes['graphql.field.name'] == 'int' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
endIf we remove the tracer module and just run the test, then this line will not find any span = spans.find { |s| s.name == 'graphql.execute_field_lazy' && s.attributes['graphql.field.name'] == 'int' }The spans look like this: |
|
Final measurements - identical to the original reported in the PR description: Warming up --------------------------------------
- small query 125.000 i/100ms
+ small query 119.000 i/100ms
large query 7.000 i/100ms
- Total allocated: 1758392 bytes (14970 objects)
+ Total allocated: 1631280 bytes (14217 objects)
Total retained: 0 bytes (0 objects)
allocated memory by gem
-----------------------------------
707840 opentelemetry-sdk-1.4.0
- 684008 graphql-1.12.24
+ 684176 graphql-1.12.24
194472 opentelemetry-api-1.2.4
- 130896 graphql/lib
41176 other
+ 3616 graphql/lib
allocated objects by gem
-----------------------------------
+ 5821 graphql-1.12.24
- 5820 graphql-1.12.24
5488 opentelemetry-sdk-1.4.0
2353 opentelemetry-api-1.2.4
- 786 graphql/lib
523 other
+ 32 graphql/lib |
|
Derp, sorry -- I forgot this was for GraphQL-Ruby 1.12.x. Here's a new script illustrating how that event might be called: Getting the execute_field_lazy event with a .trace method
require "bundler/inline"
gemfile do
gem "graphql", "~>1.12.0"
end
class MySchema < GraphQL::Schema
class Box # IRL this would probably be a Promise, Concurrent::Future, etc
def initialize(&get_value)
@get_value = get_value
end
def value
@get_value.call
end
end
class Query < GraphQL::Schema::Object
field :int, Integer, null: false
def int
puts "calling #int"
Box.new {
puts "resolving Box"
5
}
end
end
module Tracer
def self.trace(event, data)
case event
when "execute_field"
puts "Tracer -> execute_field"
when "execute_field_lazy"
puts "Tracer -> execute_field_lazy"
end
yield
end
end
lazy_resolve Box, :value
query(Query)
tracer(Tracer)
end
pp MySchema.execute("{ int }").to_h
# Tracer -> execute_field
# calling #int
# Tracer -> execute_field_lazy
# resolving Box
# {"data"=>{"int"=>5}} |
arielvalentin
changed the title
|
Thanks @rmosolgo! Looks like we need to search through events rather than spans as the span list doesn't contain graphql_tracer_test.rbit 'includes attributes using platform types - lazy' do
skip if uses_platform_interfaces?
expected_attributes = {
'graphql.field.parent' => 'Box', # type name, not interface
'graphql.field.name' => 'int',
'graphql.lazy' => false
}
BoxGraphQLAppSchema.execute('{ int }')
require 'pry'; binding.pry
span = spans.find { |s| s.name == 'graphql.execute_field_lazy' && s.attributes['graphql.field.name'] == 'int' }
_(span).wont_be_nil
_(span.attributes.to_h).must_equal(expected_attributes)
end
But the tracer can see it:
|
|
I've pushed a final commit to appease Rubocop, and will do the increased test coverage as a separate PR. |
| attrs['graphql.operation.name'] = data[:query].selected_operation_name || 'anonymous' | ||
| attrs.freeze | ||
| else | ||
| {} |
There was a problem hiding this comment.
I think this should be a frozen hash and a constant. That will mean only allocating one hash for all "misses"
elenatanasoiu
force-pushed
the
elena/reduce-objects-for-fun-and-profit
branch
from
a64f78f to
107bb1b
Compare
Context
Backporting this to
graphql-1.12.24.Closes: #865
graphql.field.parentis now reporting the interface instead of the concrete class forexecute_field. We've made this choice consciously in order to benefit from the very serious performance gains vs 100% accuracy. For graphql v2, this goes back to reporting the concrete class so we always have the choice of upgrading.graphql.operation.nameisnil, we are now setting the value toanonymous. This allows us to detect unowned queries.Results
This will result in:
otel_bench.rb
I've had to add
legacy_tracing:truebefore comparing the two benchmarks.I ran this on the main branch and then on my branch.
For the main branch, I got a lot of lines of error related to this issue I filed earlier in the week:
The fix was actually fairly straightforward:
becomes