DataDog / dd-trace-js

JavaScript APM Tracer
https://docs.datadoghq.com/tracing/
Other
643 stars 304 forks source link

GraphQL Lists deeper down #169

Closed fragglebob closed 6 years ago

fragglebob commented 6 years ago

How deep down is GraphQL supposed to instrument?

With a query like the following only these fields friends, friends.0.name, friends.0.pets, friends.1.name and friends.1.pets get instrumented. Should all the fields on the types pet and color, get instrumented too?

{
  friends {
    name
    pets {
      name
      colors {
        code
      }
    }
  }
}
rochdev commented 6 years ago

Lists have been problematic and will be fixed in the next release.

The new behavior for the integration in general will be:

• every operation, either query or mutation, gets a span • every field gets a span that includes the duration of its resolver and all child fields, but only if is has an associated resolver • every resolver gets a span that includes only its own duration • any field that doesn't have an explicit resolver, either on a programmatic schema or on the rootValue, will not be instrumented

So basically, any field that matches those criteria would get instrumented. For example, if you have a resolver on friends and code, the fields friends and friends.0.pets.0.colors.0.code would be instrumented.

The reasoning behind this behavior is that for example, if there is a list of 1000 friends, each of which has 3 pets with 3 colors each, you would get 9000 spans for a single operation. This would both incur a very high overhead and be too verbose in the UI.

Will this work for your use case?

fragglebob commented 6 years ago

I've been using the version currently in the master branch, as I get lots of errors without your fix for looping for ancestors in #167.

I've made a fork to test getting all the fields and resolves in.

In doing so I have found another issue with recursive types, for example where pets then have an owner. This would max out the call stack.

rochdev commented 6 years ago

I'll take a look at the recursion error.

Otherwise, does the behavior mentioned above match what you are seeing? And does it make sense for your use case?

fragglebob commented 6 years ago

I've made a fork and I've got these things sorted in it, I didn't want to make a PR though until I knew you want to instrument that deep.

rochdev commented 6 years ago

PR for the recursion problem: https://github.com/DataDog/dd-trace-js/pull/170

rochdev commented 6 years ago

Just saw your branch, I guess you beat me to it :)

rochdev commented 6 years ago

@fragglebob Feel free to open the PR, we do want to instrument as deep as possible whenever there are custom resolvers. If this causes a cardinality issue we will simply add a configuration option.

fragglebob commented 6 years ago

Ok will do. 😉

fragglebob commented 6 years ago

Okay my PR for both issues is here: #171

rochdev commented 6 years ago

Fixed by #171