Directives support

[lanadz-shopify]

This PR adds possibility to track directives along with their arguments.

The approach taken - add on_enter_directive to analyzer in similar way as it is done for fields.

Extended metrics result with directives key that holds directives names:

{   
  directive_name: "customDirective",  
}

and for arguments new field is added and some are renamed, since arguments can be applied not only to fields but to directives too:

Arguments

for fields

query example:

query PostDetails($postId: ID!, $titleUpcase: Boolean = false, $commentsTags: [String!] = null) {
    post(id: $postId) {
        comments(ids: [1, 2], tags: $commentsTags) { # 2 seconds recorded
           id
           body
        }
    }
}

arguments for field comments

{ 
  argument_name: "id",   
  argument_name: "ids",    
  argument_type_name: "ID",    
  parent_name: "comments",    # name of the node that argument was applied to 
  grandparent_type_name: "Post",   # grandparent node to uniquely identify which node the argument was applied to 
  grandparent_node_name: "post",    # name of grandparend node
  default_used: false,    
  value_is_null: false
}

for directives

example

query PostDetails($postId: ID!, $val: Int!) @customDirective(val: $val) {
    post(id: $postId) {
        id
        title @skip(if: true)
    }
}

in case of directive applied to field it'll look like

{
  argument_name: "if",
  argument_type_name: "Boolean",
  parent_name: "skip",
  grandparent_type_name: "__Directive",  # i couldn't get type of the node here, Chris suggested `__Directive`
  grandparent_node_name: "title",
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: SomeArgumentValue.new
}

Directive applied to query level:

{
  argument_name: "val",
  argument_type_name: "Int",
  parent_name: "customDirective",
  grandparent_type_name: "__Directive",
  grandparent_node_name: "query",
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: SomeArgumentValue.new
}

[lanadz-shopify]

Remove deprecation warnings

DEPRECATION WARNING: GraphQL::Execution::Interpreter is now the default; remove `use GraphQL::Execution::Interpreter` from the schema definition (/Users/svitlanadzyuban/src/github.com/Shopify/graphql-metrics/test/unit/graphql/metrics/integration_test.rb:84:in `<class:SchemaWithFullMetrics>') (called from <class:SchemaWithFullMetrics> at /Users/svitlanadzyuban/src/github.com/Shopify/graphql-metrics/test/unit/graphql/metrics/integration_test.rb:84)
DEPRECATION WARNING: GraphQL::Analysis::AST is now the default; remove `use GraphQL::Analysis::AST` from the schema definition (/Users/svitlanadzyuban/src/github.com/Shopify/graphql-metrics/test/unit/graphql/metrics/integration_test.rb:85:in `<class:SchemaWithFullMetrics>') (called from <class:SchemaWithFullMetrics> at /Users/svitlanadzyuban/src/github.com/Shopify/graphql-metrics/test/unit/graphql/metrics/integration_test.rb:85)

[chrisbutcher]

Thanks for the contribution!

One question re: directive arguments and otherwise minor styling suggestions.

[lanadz-shopify]

Thank you @chrisbutcher for review! I'll definitely fix those style issues. Directive arguments is a good idea to track!

[eapache]

visitor.field_definition is nil for QUERY level directive and failing for FIELD level too.

That would make sense given you're not currently analyzing a field, you're analyzing a directive. Don't you want visitor.directive_definition instead? Otherwise I think extract_arguments should work for you.

[lanadz-shopify]

@eapache, @chrisbutcher directive_definition worked perfectly, but with this caveat:

field_definition and directive_definition will be different type here (maybe I should change the name of variable to definition?)

For directive_definition it will be class and for field_definition we will have instance here, looks like it's coming from here
https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/analysis/ast/visitor.rb#L103L130 (line 103 and 130)

owner is defined on instance in graphql-query gem for Field and for Directive

Question: If we keep it as it is, should we return "Directive" instead of nil?

[eapache]

1. Yeah we should probably rename this to just definition.
2. We should probably rename a few of the metrics too (e.g. parent_field_name isn't entirely correct when the parent is a directive). This becomes a breaking change, but we're bumping the minimum graphql gem version anyway so I think we're due.
3. The intent with parent_field_name and parent_field_type_name was to be able to uniquely identify where in the query the argument was being used... perhaps we should put the name of whatever the directive is attached to instead? We might have to give this a bit more thought, if we can afford the time right now. But ultimately you're the ones using this data so as long as it provides what you need in the short term.

[lanadz-shopify]

I was thinking, maybe to keep compatibility, instead of renaming parent_field_name and parent_field_type_name we could add parent_of_directive_name and parent_of_directive_type_name and keep parent_field_name and parent_field_type_name with nil for directives (and parent_of_directive_name and parent_of_directive_type_name set to nil in case if arguments belong to fields)

[chrisbutcher]

Sketching another potential future state, with a slightly modified Star Wars API example:

query Hero($episode: Episode, $withFriends: Boolean!) {
  hero(episode: $episode) {
    name
    friends(first: 10) @include(if: $withFriends) { # 👀 
      name
    }
  }
}

For this query, looking only at the argument metrics we might gather for friends(first: 10) @include(if: $withFriends)

# Metrics for hero.friends.first
static_metrics = {
  argument_name: "first",
  argument_type_name: "Integer",
  parent_name: "friends",
  parent_type_name: "Friend",
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: 10,
}

# Metrics for hero.friends.@include.if
static_metrics = {
  argument_name: "if",
  argument_type_name: "Boolean",
  parent_name: "include",
  parent_type_name: "__Directive",
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: false,
}

Perhaps just renaming to parent_name and parent_type_name with the convention of emitting reserved __-prefix type names for built-in types would be a reliable way for applications to distinguish directive arguments from field arguments. What do you both think?

[lanadz-shopify]

Thank you @chrisbutcher.
I like idea having "__Directive", though not sure if it'll be enough to identify it (for example we could have @include applied to multiple fields)

query Hero($episode: Episode, $withFriends: Boolean!) {
  hero(episode: $episode) {
    name @include(if: true)
    friends(first: 10) @include(if: $withFriends) { # 👀 
      name @include(if: true)
    }
  }
}

then

# Metrics for hero.friends.@include.if
static_metrics = {
  argument_name: "if",
  argument_type_name: "Boolean",
  parent_name: "include",
  parent_type_name: "__Directive",
  parent_node_parent_name: "friends",      # <----- maybe?
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: false,
}

# Metrics for hero.name.@include.if
static_metrics = {
  argument_name: "if",
  argument_type_name: "Boolean",
  parent_name: "include",
  parent_type_name: "__Directive",
  parent_node_parent_name: "name",      # <----- maybe?
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: false,
}

# Metrics for hero.friends.name.@include.if
static_metrics = {
  argument_name: "if",
  argument_type_name: "Boolean",
  parent_name: "include",
  parent_type_name: "__Directive",
  parent_node_parent_name: "name",      # <----- maybe?  !!! same as for hero name
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: false,
}

Though looks like your example will provide different metrics for hero.friends.first. It will be

static_metrics = {
  argument_name: "first",
  argument_type_name: "Integer",
  parent_name: "friends",
  parent_type_name: "QueryRoot",      #  <----- this
  parent_input_object_type: nil,
  default_used: false,
  value_is_null: false,
  value: 10,
}

and the reason is the way we handle arguments currently (i also found it confusing)
Here https://github.com/Shopify/graphql-metrics/blob/master/lib/graphql/metrics/analyzer.rb#L125l126 we have

 parent_field_name: field_defn.graphql_name,         
 parent_field_type_name: field_defn.owner.graphql_name,

from conversation with @eapache

The name of parent_field_type_name is a bit confusing but using owner is intentional. We wanted to be able to uniquely identify the specific field. There might be other fields on other types named post which return Post, but there's only ever one field named post on the specific type (QueryRoot)

[eapache]

1. Yeah we should probably rename this to just definition.
2. We should probably rename a few of the metrics too (e.g. parent_field_name isn't entirely correct when the parent is a directive). This becomes a breaking change, but we're bumping the minimum graphql gem version anyway so I think we're due.
3. The intent with parent_field_name and parent_field_type_name was to be able to uniquely identify where in the query the argument was being used... perhaps we should put the name of whatever the directive is attached to instead? We might have to give this a bit more thought, if we can afford the time right now. But ultimately you're the ones using this data so as long as it provides what you need in the short term.

[eapache]

It was a little unclear to me at first which field would return which value in each case, but it became clear on reading the tests. Might be worth a clearer in-code doc or README update though.

[chrisbutcher]

Thank you! 🎉

Just a few minor suggestions at this point.

[chrisbutcher]

As mentioned on Slack, this is a major breaking change, so we'll want to bump the gem to 5.0.0 once this branch/the merge commit are tested against Shopify Core CI.