GraphQL Cleanup (#290)

* GraphQL cleanup changes

* Fix broken tests

* Add ignored fields and introspection fields

* Clean up to_source object function

* Clean up existing exception tests

Co-authored-by: lrafeei <[email protected]>

* Fix default values in trace

* Fix logic for parsing and transaction naming

* Basic async test

*  

Fix failing tests and update txn group.

* Update non-null resolver test.

* Update non-null error case.

* Seperate async testing to new file.

* Drop support for graphql 2.1 (3 year support policy).

* Remove 2.1 from test matrix.

Co-authored-by: lrafeei <[email protected]>
Co-authored-by: Uma Annamalai <[email protected]>

Author: 3 people

newrelic/api/graphql_trace.py

@@ -29,9 +29,9 @@ def __init__(self, **kwargs):
             parent = kwargs['parent']
         super(GraphQLOperationTrace, self).__init__(parent)
 
-        self.operation_name = None
-        self.operation_type = None
-        self.deepest_path = None
+        self.operation_name = "<anonymous>"
+        self.operation_type = "<unknown>"
+        self.deepest_path = "<unknown>"
         self.graphql = None
         self.graphql_format = None
         self.statement = None
@@ -44,7 +44,7 @@ def formatted(self):
         if not self.statement:
             return "<unknown>"
 
-        transaction = current_transaction()
+        transaction = current_transaction(active_only=False)
 
         # Record SQL settings
         settings = transaction.settings
@@ -55,9 +55,9 @@ def formatted(self):
 
     def finalize_data(self, transaction, exc=None, value=None, tb=None):
         # Add attributes
-        self._add_agent_attribute("graphql.operation.type", self.operation_type or "<unknown>")
-        self._add_agent_attribute("graphql.operation.name", self.operation_name or "<anonymous>")
-        self._add_agent_attribute("graphql.operation.deepestPath", self.deepest_path or "<unknown>")
+        self._add_agent_attribute("graphql.operation.type", self.operation_type)
+        self._add_agent_attribute("graphql.operation.name", self.operation_name)
+        self._add_agent_attribute("graphql.operation.deepestPath", self.deepest_path)
 
         # Attach formatted graphql
         limit = transaction.settings.agent_limits.sql_query_length_maximum

newrelic/config.py

@@ -2407,6 +2407,11 @@ def _process_module_builtin_defaults():
         "newrelic.hooks.framework_graphql",
         "instrument_graphql_error_located_error",
     )
+    _process_module_definition(
+        "graphql.language.parser",
+        "newrelic.hooks.framework_graphql",
+        "instrument_graphql_parser",
+    )
     _process_module_definition(
         "graphql.validation.validate",
         "newrelic.hooks.framework_graphql",

newrelic/core/graphql_node.py

@@ -96,9 +96,9 @@ def time_metrics(self, stats, root, parent):
 class GraphQLOperationNode(_GraphQLOperationNode, GraphQLNodeMixin):
     @property
     def name(self):
-        operation_type = self.operation_type or "<unknown>"
-        operation_name = self.operation_name or "<anonymous>"
-        deepest_path = self.deepest_path or "<unknown>"
+        operation_type = self.operation_type
+        operation_name = self.operation_name
+        deepest_path = self.deepest_path
         product = self.product
 
         name = 'GraphQL/operation/%s/%s/%s/%s' % (product, operation_type,
@@ -112,9 +112,9 @@ def time_metrics(self, stats, root, parent):
 
         """
 
-        operation_type = self.operation_type or "<unknown>"
-        operation_name = self.operation_name or "<anonymous>"
-        deepest_path = self.deepest_path or "<unknown>"
+        operation_type = self.operation_type
+        operation_name = self.operation_name
+        deepest_path = self.deepest_path
         product = self.product
 
         # Determine the scoped metric

newrelic/hooks/framework_graphql.py

@@ -12,18 +12,22 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from newrelic.api.error_trace import ErrorTrace, ErrorTraceWrapper
-from newrelic.api.function_trace import FunctionTrace, FunctionTraceWrapper
+from newrelic.api.error_trace import ErrorTrace
+from newrelic.api.function_trace import FunctionTrace
 from newrelic.api.graphql_trace import GraphQLResolverTrace, GraphQLOperationTrace
 from newrelic.api.time_trace import notice_error, current_trace
-from newrelic.api.transaction import current_transaction
+from newrelic.api.transaction import current_transaction, ignore_transaction
 from newrelic.common.object_names import callable_name, parse_exc_info
 from newrelic.common.object_wrapper import function_wrapper, wrap_function_wrapper
 from newrelic.core.graphql_utils import graphql_statement
 from collections import deque
 from copy import copy
 
 
+GRAPHQL_IGNORED_FIELDS = frozenset(("id", "__typename"))
+GRAPHQL_INTROSPECTION_FIELDS = frozenset(("__schema", "__type"))
+
+
 def graphql_version():
     from graphql import __version__ as version
     return tuple(int(v) for v in version.split("."))
@@ -45,7 +49,7 @@ def ignore_graphql_duplicate_exception(exc, val, tb):
             _, _, fullnames, message = parse_exc_info((None, val.original_error, None))
             fullname = fullnames[0]
             for error in transaction._errors:
-                if error.type == fullname and error.message == message:
+                if error.message == message:
                     return True
 
     return None  # Follow original exception matching rules
@@ -76,39 +80,48 @@ def bind_operation_v2(exe_context, operation, root_value):
 
 
 def wrap_execute_operation(wrapped, instance, args, kwargs):
+    transaction = current_transaction()
     trace = current_trace()
 
-    if trace:
+    if not transaction:
+        return wrapped(*args, **kwargs)
+
+    try:
+        operation = bind_operation_v3(*args, **kwargs)
+    except TypeError:
+        operation = bind_operation_v2(*args, **kwargs)
+
+    operation_name = operation.name
+    if hasattr(operation_name, "value"):
+        operation_name = operation_name.value
+    trace.operation_name = operation_name = operation_name or "<anonymous>"
+
+    operation_type = operation.operation
+    if hasattr(operation_type, "name"):
+        operation_type = operation_type.name.lower()
+    trace.operation_type = operation_type = operation_type or "<unknown>"
+
+    if operation.selection_set is not None:
+        fields = operation.selection_set.selections
         try:
-            operation = bind_operation_v3(*args, **kwargs)
-        except TypeError:
-            operation = bind_operation_v2(*args, **kwargs)
-
-        operation_name = operation.name
-        if hasattr(operation_name, "value"):
-            operation_name = operation_name.value
-        trace.operation_name = operation_name
-
-        operation_type = operation.operation
-        if hasattr(operation_type, "name"):
-            operation_type = operation_type.name
-        trace.operation_type = operation_type.lower()
-
-        if operation.selection_set is not None:
-            fields = operation.selection_set.selections
-            try:
-                deepest_path = traverse_deepest_path(fields)
-            except:
-                deepest_path = []
-            trace.deepest_path = ".".join(deepest_path)
+            deepest_path = traverse_deepest_path(fields)
+        except:
+            deepest_path = []
+        trace.deepest_path = deepest_path = ".".join(deepest_path) or "<unknown>"
 
-    return wrapped(*args, **kwargs)
+    transaction.set_transaction_name(callable_name(wrapped), "GraphQL", priority=11)
+
+    result = wrapped(*args, **kwargs)
+    transaction_name = "%s/%s/%s" % (operation_type, operation_name, deepest_path)
+    transaction.set_transaction_name(transaction_name, "GraphQL", priority=14)
+
+    return result
 
 
 def traverse_deepest_path(fields):
     inputs = deque(((field, deque(), 0) for field in fields))
     deepest_path_len = 0
-    deepest_path = None
+    deepest_path = []
 
     while inputs:
         field, current_path, current_path_len = inputs.pop()
@@ -117,16 +130,23 @@ def traverse_deepest_path(fields):
         if hasattr(field_name, "value"):
             field_name = field_name.value
 
-        current_path.append(field_name)
-        current_path_len += 1
-
-        if field.selection_set is None or len(field.selection_set.selections) == 0:
-            if deepest_path_len < current_path_len:
-                deepest_path = current_path
-                deepest_path_len = current_path_len
-        else:
-            for inner_field in field.selection_set.selections:
-                inputs.append((inner_field, copy(current_path), current_path_len))
+        if field_name in GRAPHQL_INTROSPECTION_FIELDS:
+            # If an introspection query is identified, ignore the transaction completely.
+            # This matches the logic used in Apollo server for identifying introspection queries.
+            ignore_transaction()
+            return []
+        elif field_name not in GRAPHQL_IGNORED_FIELDS:
+            # Only add to the current path for non-ignored values.
+            current_path.append(field_name)
+            current_path_len += 1
+
+            if field.selection_set is None or len(field.selection_set.selections) == 0:
+                if deepest_path_len < current_path_len:
+                    deepest_path = current_path
+                    deepest_path_len = current_path_len
+            else:
+                for inner_field in field.selection_set.selections:
+                    inputs.append((inner_field, copy(current_path), current_path_len))
 
     return deepest_path
 
@@ -137,12 +157,25 @@ def bind_get_middleware_resolvers(middlewares):
 
 def wrap_get_middleware_resolvers(wrapped, instance, args, kwargs):
     middlewares = bind_get_middleware_resolvers(*args, **kwargs)
-    middlewares = [FunctionTraceWrapper(ErrorTraceWrapper(m, ignore=ignore_graphql_duplicate_exception)) if not hasattr(m, "_nr_wrapped") else m for m in middlewares]
+    middlewares = [wrap_middleware(m) if not hasattr(m, "_nr_wrapped") else m for m in middlewares]
     for m in middlewares:
         m._nr_wrapped = True
 
     return wrapped(middlewares)
 
+@function_wrapper
+def wrap_middleware(wrapped, instance, args, kwargs):
+    transaction = current_transaction()
+    if transaction is None:
+        return wrapped(*args, **kwargs)
+
+    name = callable_name(wrapped)
+    transaction.set_transaction_name(name, 'GraphQL', priority=12)
+    with FunctionTrace(name):
+        with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
+            return wrapped(*args, **kwargs)
+
+
 
 def bind_get_field_resolver(field_resolver):
     return field_resolver
@@ -182,14 +215,11 @@ def wrap_executor_execute(wrapped, instance, args, kwargs):
 @function_wrapper
 def wrap_resolver(wrapped, instance, args, kwargs):
     transaction = current_transaction()
-    # Prevent double wrapping using _nr_wrapped attr
     if transaction is None:
         return wrapped(*args, **kwargs)
-
-    transaction.set_transaction_name(callable_name(wrapped), priority=2)
-    with FunctionTrace(callable_name(wrapped)):
-        with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
-            return wrapped(*args, **kwargs)
+    transaction.set_transaction_name(callable_name(wrapped), "GraphQL", priority=13)
+    with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
+        return wrapped(*args, **kwargs)
 
 
 def wrap_error_handler(wrapped, instance, args, kwargs):
@@ -198,6 +228,13 @@ def wrap_error_handler(wrapped, instance, args, kwargs):
 
 
 def wrap_validate(wrapped, instance, args, kwargs):
+    transaction = current_transaction()
+    if transaction is None:
+        return wrapped(*args, **kwargs)
+
+    transaction.set_transaction_name(callable_name(wrapped),"GraphQL", priority=10)
+
+    # Run and collect errors
     errors = wrapped(*args, **kwargs)
 
     # Raise errors and immediately catch them so we can record them
@@ -209,6 +246,15 @@ def wrap_validate(wrapped, instance, args, kwargs):
 
     return errors
 
+def wrap_parse(wrapped, instance, args, kwargs):
+    transaction = current_transaction()
+    if transaction is None:
+        return wrapped(*args, **kwargs)
+    transaction.set_transaction_name(callable_name(wrapped), "GraphQL", priority=10)
+
+    with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
+        return wrapped(*args, **kwargs)
+
 
 def bind_resolve_field_v3(parent_type, source, field_nodes, path):
     return parent_type, field_nodes, path
@@ -233,13 +279,14 @@ def wrap_resolve_field(wrapped, instance, args, kwargs):
     field_name = field_asts[0].name.value
 
     with GraphQLResolverTrace(field_name) as trace:
-        trace._add_agent_attribute("graphql.field.parentType", parent_type.name)
-        if isinstance(field_path, list):
-            trace._add_agent_attribute("graphql.field.path", field_path[0])
-        else:
-            trace._add_agent_attribute("graphql.field.path", field_path.key)
+        with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
+            trace._add_agent_attribute("graphql.field.parentType", parent_type.name)
+            if isinstance(field_path, list):
+                trace._add_agent_attribute("graphql.field.path", field_path[0])
+            else:
+                trace._add_agent_attribute("graphql.field.path", field_path.key)
 
-        return wrapped(*args, **kwargs)
+            return wrapped(*args, **kwargs)
 
 
 def bind_graphql_impl_query(schema, source, *args, **kwargs):
@@ -280,10 +327,14 @@ def wrap_graphql_impl(wrapped, instance, args, kwargs):
     if hasattr(query, "body"):
         query = query.body
 
+    transaction.set_transaction_name(callable_name(wrapped), "GraphQL", priority=10)
+
     with GraphQLOperationTrace() as trace:
         trace.statement = graphql_statement(query)
         with ErrorTrace(ignore=ignore_graphql_duplicate_exception):
-            return wrapped(*args, **kwargs)
+            result = wrapped(*args, **kwargs)
+            #transaction.set_transaction_name(transaction_name, "GraphQL", priority=14)
+            return result
 
 
 def instrument_graphql_execute(module):
@@ -336,8 +387,13 @@ def instrument_graphql_error_located_error(module):
 def instrument_graphql_validate(module):
     wrap_function_wrapper(module, "validate", wrap_validate)
 
+
 def instrument_graphql(module):
     if hasattr(module, "graphql_impl"):
         wrap_function_wrapper(module, "graphql_impl", wrap_graphql_impl)
     if hasattr(module, "execute_graphql"):
         wrap_function_wrapper(module, "execute_graphql", wrap_graphql_impl)
+
+
+def instrument_graphql_parser(module):
+    wrap_function_wrapper(module, "parse", wrap_parse)

tests/framework_graphql/_target_application.py

@@ -25,10 +25,11 @@
 
 libraries = [
     {
+        "id": 1,
         "name": "NYC Public Library",
-        "book": [{"name": "A", "author": "B"}, {"name": "C", "author": "D"}],
+        "book": [{"name": "A", "author": "B", "id": 1}, {"name": "C", "author": "D", "id": 2}],
     },
-    {"name": "Portland Public Library", "book": [{"name": "E", "author": "F"}]},
+    {"id": 2, "name": "Portland Public Library", "book": [{"name": "E", "author": "F", "id": 1}]},
 ]
 
 storage = []
@@ -48,6 +49,7 @@ def resolve_storage(parent, info):
 Book = GraphQLObjectType(
     "Book",
     {
+        "id": GraphQLField(GraphQLInt),
         "name": GraphQLField(GraphQLString),
         "author": GraphQLField(GraphQLString),
     },
@@ -56,6 +58,7 @@ def resolve_storage(parent, info):
 Library = GraphQLObjectType(
     "Library",
     {
+        "id": GraphQLField(GraphQLInt),
         "name": GraphQLField(GraphQLString),
         "book": GraphQLField(GraphQLList(Book)),
     },

tests/framework_graphql/conftest.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import pytest
+import six
 from testing_support.fixtures import (
     code_coverage_fixture,
     collector_agent_registration_fixture,
@@ -44,3 +45,9 @@ def app():
     from _target_application import _target_application
 
     return _target_application
+
+
+if six.PY2:
+    collect_ignore = [
+        'test_application_async.py'
+    ]