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.

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

chore: Add Tests for GraphQL C Parser #773

Merged
merged 6 commits into from
Dec 22, 2023

Conversation

arielvalentin
Copy link
Collaborator

@arielvalentin arielvalentin commented Dec 22, 2023

Graphql 2.2 introduces an optional C implementation of the Parser as well as optimizations to the pure Ruby parser that result in changes to what is captured during instrumentation.

Users of the pure Ruby implementation will no longer see graphql.lex spans emitted as a part of their traces and will instead need to switch to using the C Parser in order to explicitly see time spent in the lexer.

This PR introduces updates to the test suite to add coverage for the C Parser and changes to the Ruby implementation.

There are no underlying changes in the instrumentation itself.

@ravangen
Copy link
Contributor

If you do want tests to run on both the default parser, and the c parser, I do something similar with:

USING_C_PARSER = T.let(false, T::Boolean)
if ENV["GRAPHQL_CPARSER"] == "1"
  require "graphql/c_parser"
  USING_C_PARSER = true
end

# When tracing, is the parser expected to call `lex` before `parse`
def trace_lex_supported?
  return @trace_lex_supported if defined?(@trace_lex_supported)

  # In GraphQL 2.2, the default parser was changed such that `lex` is no longer called
  @trace_lex_supported = USING_C_PARSER ||
    Gem::Requirement.new("< 2.2").satisfied_by?(Gem::Version.new(GraphQL::VERSION))
end

Where I have CI run the appraisals twice (GRAPHQL_CPARSER=0 and GRAPHQL_CPARSER=1) since I wasn't sure how to put that into the appraisal itself.

@arielvalentin
Copy link
Collaborator Author

Thanks for the pointers.

@arielvalentin
Copy link
Collaborator Author

@ravangen @rmosolgo @swalkinshaw I added separate appraisals for the C Parser and I think I found the incantation necessary to not have to rely on setting environment variables for our test suite.

Please take a look and let me know if these changes:

  1. Are sensible
  2. Are maintainable
  3. Provide the appropriate coverage for the functionality that includes the C Parser

Thank you.

Comment on lines +32 to +33
appraise 'graphql-2.2.x' do
gem 'graphql', '~> 2.2.1', '< 3.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want to have one run against latest?

Perhaps could match the styles of other appraisals to the same effect.

Suggested change
appraise 'graphql-2.2.x' do
gem 'graphql', '~> 2.2.1', '< 3.0.0'
# Max compatible version of 2.2.x
appraise 'graphql-2.2' do
gem 'graphql', '~> 2.2.1'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Major version changes have been difficult for us to test using appraisals, while maintaining minor versions.

We used to have canary builds that rain on unpinned versions of gems but I couldn't keep up with them so I disabled those builds.

I will however add unpinned versions of the gem to this PR and see if that puts us in a better place for the future canary work.

Copy link
Collaborator Author

@arielvalentin arielvalentin Dec 22, 2023

Choose a reason for hiding this comment

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

Added here: 4451a54


# In GraphQL 2.2, the default parser was changed such that `lex` is no longer called
@trace_lex_supported = Gem::Requirement.new('< 2.2').satisfied_by?(Gem::Version.new(GraphQL::VERSION)) \
|| (defined?(GraphQL::CParser) == 'constant')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anything result in require "graphql/c_parser" such that this is true when the gem is installed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The test helper uses bundler require, which will load all of the declared gems by default before running any tests.

In this case the Gemfile requires the graphql-c_parser file in the gems' lib directory, which requires graphql/c_parser... then turtles all the way down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arielvalentin arielvalentin changed the title fix: GraphQL 2.2 incompatabilities chore: Add Tests for GraphQL C Parser Dec 22, 2023
Copy link
Contributor

@kaylareopelle kaylareopelle left a comment

Choose a reason for hiding this comment

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

Wow! Nice work. I learned a lot from this PR.

@@ -103,7 +105,7 @@
it 'traces the provided schemas' do
SomeOtherGraphQLAppSchema.execute('query SimpleQuery{ __typename }')

_(spans.size).must_equal(8)
_(spans.select { |s| s.name.start_with?('graphql.') }).wont_be(:empty?)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like looking for the name rather than the quantity. Nice update.

@arielvalentin arielvalentin requested a review from simi as a code owner December 22, 2023 21:29
@arielvalentin arielvalentin requested review from a team December 22, 2023 21:29
@arielvalentin arielvalentin merged commit 2586b75 into open-telemetry:main Dec 22, 2023
47 checks passed
@arielvalentin arielvalentin deleted the fix-graphql-2.2 branch December 22, 2023 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants