Add GraphQL configuration #295

delner · 2018-01-04T23:35:58Z

Adds graphql as an available integration for configuration.

To enable GraphQL tracing, add the following to the DataDog initializer or equivalent:

Datadog.configure do |c|
  c.use :graphql,
        service_name: 'graphql',
        tracer: Datadog.tracer,
        schemas: [YourSchema]
end

Options available:

service_name: (optional, default ruby-graphql) Name of the service that should appear on traces.
tracer: (optional, default Datadog.tracer) Instance of Datadog::Tracer to use.
schemas: (default []) Array of GraphQL::Schema objects which to trace. use will add tracing to all the schemas listed, using the same options provided above. If you supply your schema in this option, do not add use(GraphQL::Tracing::DataDogTracing) to the schema's definition, to avoid double traces. If you do not supply this option, you will need to individually configure your own schemas (see below.)

If you have multiple schemas, and you prefer to individually configure the tracer settings for each,
in the schema definition, you can add the following:

YourSchema = GraphQL::Schema.define do
  use(
    GraphQL::Tracing::DataDogTracing,
    service: 'graphql',
    tracer: Datadog.tracer
  )
end

Or you can modify an already defined schema:

YourSchema.define do
  use(
    GraphQL::Tracing::DataDogTracing,
    service: 'graphql',
    tracer: Datadog.tracer
  )
end

The options are optional, with the same defaults as defined above.

You may want to exercise this option if you have multiple schemas, and you want to give them different service names, modify how their tracers work, etc... If you do this however, do not list the schema in the schemas option for use :graphql.

NOTE: GraphQL tests are expected to fail right now, as this PR depends upon rmosolgo/graphql-ruby#1208 for a bugfix and support for tracer configuration. Tests should pass after GraphQL is updated.

delner · 2018-01-05T18:34:45Z

Rakefile

    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:faraday'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:aws'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:mongodb'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:sucker_punch'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:dalli'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:resque'
+    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:racecar'


We evidently weren't running racecar tests in CI? Whoops.

@delner if this PR should wait something else before the merge, remove this line and create a new PR so we can merge this fix immediately. Thank you!

PR for this fix is here: #298

p-lambert

just a minor a change and we're good to go. I think that's exactly what we need, so great job 👍

p-lambert · 2018-01-05T19:41:02Z

Rakefile

    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:faraday'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:aws'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:mongodb'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:sucker_punch'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:dalli'
    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:resque'
+    sh 'rvm $MRI_VERSIONS --verbose do appraisal contrib rake test:racecar'


p-lambert · 2018-01-05T19:42:42Z

lib/ddtrace/contrib/graphql/patcher.rb

+        include Base
+        register_as :graphql
+        option :tracer, default: Datadog.tracer
+        option :service_name, default: 'ruby-graphql' do |value|


you should add here: depends_on: [:tracer]. that way we can ensure tracer option will be evaluated first

p-lambert · 2018-01-05T19:44:57Z

lib/ddtrace/contrib/graphql/patcher.rb

+
+          private
+
+          def configuration


delner · 2018-01-29T15:28:34Z

The parent PR rmosolgo/graphql-ruby#1208 has been merged, and should roll with graph-ql version 1.7.9. Will update this PR then.

delner · 2018-02-01T20:01:18Z

Necessary changes were released with graphql versions 1.7.9 and 1.8.0.pre5. Updated this pull request accordingly.

delner · 2018-02-01T20:02:50Z

Probably should add documentation for this.

palazzem · 2018-02-05T16:37:01Z

lib/ddtrace/contrib/graphql/patcher.rb

+          private
+
+          def compatible?
+            defined?(::GraphQL) && defined?(::GraphQL::Tracing::DataDogTracing)


we should also check the version if 1.7.9+, otherwise it's not going to work or work as we expect after rmosolgo/graphql-ruby#1060 merge

Feedback addressed.

palazzem · 2018-02-05T20:37:01Z

docs/GettingStarted.md

+  use(
+    GraphQL::Tracing::DataDogTracing,
+    service: 'graphql',
+    tracer: Datadog.tracer


I would remove tracer from the example. Let's keep it minimal.

This is pretty important if you're manually instrumenting. I'd recommend keeping it especially since its not documented anywhere else in either ddtrace or graphql. But if you're convinced it doesn't add value, I'll remove it.

Datadog.tracer is always the de-facto tracer you have to use. Initializing a new tracer and set it as tracer for GraphQL (while using other integrations) will only create instrumentation issues.

I'll remove this.

palazzem · 2018-02-05T20:38:56Z

docs/GettingStarted.md

+| Key | Description | Default |
+| --- | --- | --- |
+| ``service_name`` | Service name used for `graphql` instrumentation | ``ruby-graphql`` |
+| ``schemas`` | Optional. Array of `GraphQL::Schema` objects which to trace. Tracing will be added to all the schemas listed, using the options provided to this configuration. If you supply your schema in this option, do not add `use(GraphQL::Tracing::DataDogTracing)` to the schema's definition, to avoid double traces. If you do not supply this option, you will need to individually configure your own schemas (see below.) | ``[]`` |


If schemas is optional, what happens if it's an empty array and you just:

Datadog.configure do |c| c.use :graphql, service_name: 'graphql' end

Then the integration effectively does nothing. This integration is just a convenience wrapper on top of GraphQL's tracing implementation, and as such, needs a list of schemas to patch.

You might want to do this without schemas when you want to manually instrument the schemas yourself, and consequently use this configuration as a fact table to aid that setup.

palazzem · 2018-02-05T20:44:21Z

spec/ddtrace/contrib/graphql/tracer_spec.rb

+      # Expect each span to be properly named
+      all_spans.each do |span|
+        expect(span.service).to eq('graphql-test')
+        expect(span.resource.to_s).to_not be_empty


what kind of resource are we testing here? I would prefer to be 100% sure that we expect some values rather than "it's not empty". Especially because ::GraphQL::Tracing::DataDogTracing is not maintained in this library and so would be great knowing about breaking changes.

This used to be the broken thing in graphql. It used to be empty, now it returns something. We could update that assertion to be a bit more explicit, but it's going to be different for every single span. Since graphql is doing the instrumentation here, if they change what spans they record, it'll break our CI suite.

We probably shouldn't be asserting specific side effects from GraphQL at all, only that we were able to get some spans that look like GraphQL is working still. Otherwise we could find ourselves having to fix a very brittle test anytime they change something on their end (when in fact, nothing broke at all.)

palazzem

Good to go! thank you very much!

delner added the do-not-merge/WIP Not ready for merge label Jan 4, 2018

delner self-assigned this Jan 4, 2018

palazzem added this to the 0.11.0 milestone Jan 5, 2018

palazzem requested review from palazzem and p-lambert January 5, 2018 09:21

delner force-pushed the delner/add_graphql_configuration branch 4 times, most recently from 9cc3efc to 9fb04d5 Compare January 5, 2018 18:31

delner commented Jan 5, 2018

View reviewed changes

delner force-pushed the delner/add_graphql_configuration branch from 9fb04d5 to 65d1d5f Compare January 5, 2018 18:37

p-lambert previously requested changes Jan 5, 2018

View reviewed changes

delner force-pushed the delner/add_graphql_configuration branch 3 times, most recently from cee8cd4 to 28c080c Compare January 8, 2018 18:10

palazzem removed this from the 0.11.0 milestone Jan 11, 2018

delner added this to the 0.11.1 milestone Jan 16, 2018

delner added the integrations Involves tracing integrations label Jan 19, 2018

delner modified the milestones: 0.11.1, 0.11.2 Jan 24, 2018

delner force-pushed the delner/add_graphql_configuration branch from 28c080c to 6382c92 Compare February 1, 2018 19:59

delner removed the do-not-merge/WIP Not ready for merge label Feb 1, 2018

palazzem modified the milestones: 0.11.2, 0.11.3 Feb 2, 2018

delner and others added 3 commits February 5, 2018 11:34

Added: GraphQL integration configuration.

9fb7ace

Added: GraphQL tests for basic tracing.

dd53633

Added: GraphQL to documentation

d5a5cb7

delner added 2 commits February 5, 2018 11:34

Fixed: GraphQL integration not auto-loading

3dae1b6

Added: GraphQL RSpec example

021c71c

palazzem reviewed Feb 5, 2018

View reviewed changes

delner modified the milestones: 0.11.3, 0.12.0 Feb 5, 2018

delner changed the base branch from master to 0.12-dev February 5, 2018 18:56

delner force-pushed the delner/add_graphql_configuration branch from 576433b to 021c71c Compare February 5, 2018 19:03

delner added the feature Involves a product feature label Feb 5, 2018

Added: Version constraint for GraphQL 1.7.9+

5a53d0f

Added: LogHelpers to suppress warnings from graphql

f221b26

delner force-pushed the delner/add_graphql_configuration branch from 8731d67 to f221b26 Compare February 5, 2018 20:16

palazzem suggested changes Feb 5, 2018

View reviewed changes

Changed: GraphQL schemas option and specs

bd10fbf

palazzem approved these changes Feb 7, 2018

View reviewed changes

delner merged commit 815a66a into 0.12-dev Feb 7, 2018

delner deleted the delner/add_graphql_configuration branch February 9, 2018 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GraphQL configuration #295

Add GraphQL configuration #295

delner commented Jan 4, 2018 •

edited by palazzem

Loading

delner Jan 5, 2018

p-lambert Jan 5, 2018

palazzem Jan 8, 2018

delner Jan 8, 2018

p-lambert left a comment

p-lambert Jan 5, 2018

p-lambert Jan 5, 2018

p-lambert Jan 5, 2018

delner commented Jan 29, 2018 •

edited

Loading

delner commented Feb 1, 2018

delner commented Feb 1, 2018

palazzem Feb 5, 2018

palazzem Feb 5, 2018

delner Feb 5, 2018

palazzem Feb 5, 2018

delner Feb 5, 2018

palazzem Feb 5, 2018

delner Feb 5, 2018 •

edited

Loading

palazzem Feb 5, 2018

delner Feb 5, 2018 •

edited

Loading

palazzem left a comment

Add GraphQL configuration #295

Add GraphQL configuration #295

Conversation

delner commented Jan 4, 2018 • edited by palazzem Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-lambert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner commented Jan 29, 2018 • edited Loading

delner commented Feb 1, 2018

delner commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delner Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

palazzem left a comment

Choose a reason for hiding this comment

delner commented Jan 4, 2018 •

edited by palazzem

Loading

delner commented Jan 29, 2018 •

edited

Loading

delner Feb 5, 2018 •

edited

Loading

delner Feb 5, 2018 •

edited

Loading