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

Add GraphQL configuration #295

Merged
merged 8 commits into from
Feb 7, 2018
Merged

Conversation

delner
Copy link
Contributor

@delner delner commented Jan 4, 2018

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 delner added the do-not-merge/WIP Not ready for merge label Jan 4, 2018
@delner delner self-assigned this Jan 4, 2018
@palazzem palazzem added this to the 0.11.0 milestone Jan 5, 2018
@palazzem palazzem requested review from palazzem and p-lambert January 5, 2018 09:21
@delner delner force-pushed the delner/add_graphql_configuration branch 4 times, most recently from 9cc3efc to 9fb04d5 Compare January 5, 2018 18:31
Rakefile Outdated
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'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR for this fix is here: #298

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

@p-lambert p-lambert left a comment

Choose a reason for hiding this comment

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

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

Rakefile Outdated
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'
Copy link
Member

Choose a reason for hiding this comment

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

👍

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

Choose a reason for hiding this comment

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

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


private

def configuration
Copy link
Member

Choose a reason for hiding this comment

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

dead code?

@delner delner force-pushed the delner/add_graphql_configuration branch 3 times, most recently from cee8cd4 to 28c080c Compare January 8, 2018 18:10
@palazzem palazzem removed this from the 0.11.0 milestone Jan 11, 2018
@delner delner added this to the 0.11.1 milestone Jan 16, 2018
@delner delner added the integrations Involves tracing integrations label Jan 19, 2018
@delner delner modified the milestones: 0.11.1, 0.11.2 Jan 24, 2018
@delner
Copy link
Contributor Author

delner commented Jan 29, 2018

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 delner force-pushed the delner/add_graphql_configuration branch from 28c080c to 6382c92 Compare February 1, 2018 19:59
@delner
Copy link
Contributor Author

delner commented Feb 1, 2018

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

@delner delner removed the do-not-merge/WIP Not ready for merge label Feb 1, 2018
@delner
Copy link
Contributor Author

delner commented Feb 1, 2018

Probably should add documentation for this.

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

def compatible?
defined?(::GraphQL) && defined?(::GraphQL::Tracing::DataDogTracing)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@delner delner modified the milestones: 0.11.3, 0.12.0 Feb 5, 2018
@delner delner changed the base branch from master to 0.12-dev February 5, 2018 18:56
@delner delner force-pushed the delner/add_graphql_configuration branch from 576433b to 021c71c Compare February 5, 2018 19:03
@delner delner added the feature Involves a product feature label Feb 5, 2018
@delner delner dismissed p-lambert’s stale review February 5, 2018 20:07

Feedback addressed.

@delner delner force-pushed the delner/add_graphql_configuration branch from 8731d67 to f221b26 Compare February 5, 2018 20:16
use(
GraphQL::Tracing::DataDogTracing,
service: 'graphql',
tracer: Datadog.tracer
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this.

| 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.) | ``[]`` |
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@delner delner Feb 5, 2018

Choose a reason for hiding this comment

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

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.

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@delner delner Feb 5, 2018

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Good to go! thank you very much!

@delner delner merged commit 815a66a into 0.12-dev Feb 7, 2018
@delner delner deleted the delner/add_graphql_configuration branch February 9, 2018 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants