From 2c83422855ad689cc845239b4d9ac8d1a4138bdc Mon Sep 17 00:00:00 2001 From: Uma Annamalai Date: Thu, 29 Jul 2021 10:12:58 -0700 Subject: [PATCH] Add field resolver returnType span attribute (#300) * Add field.returnType span attr and update error logic. * Change solr docker image to pass health checks. --- .github/workflows/tests.yml | 2 +- newrelic/core/attribute.py | 1 + newrelic/hooks/framework_graphql.py | 28 +++++++++++++------ tests/framework_graphql/test_application.py | 25 +++++++++++------ .../test_application_async.py | 6 +++- 5 files changed, 42 insertions(+), 20 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 690011a9e4..afcda00a6c 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -509,7 +509,7 @@ jobs: services: solr: - image: bitnami/solr:8 + image: bitnami/solr:8.8.2 env: SOLR_CORE: collection ports: diff --git a/newrelic/core/attribute.py b/newrelic/core/attribute.py index 474bfec83e..91dd6fd406 100644 --- a/newrelic/core/attribute.py +++ b/newrelic/core/attribute.py @@ -75,6 +75,7 @@ 'graphql.field.name', 'graphql.field.parentType', 'graphql.field.path', + 'graphql.field.returnType', 'graphql.operation.name', 'graphql.operation.type', 'graphql.operation.query', diff --git a/newrelic/hooks/framework_graphql.py b/newrelic/hooks/framework_graphql.py index a321fd75bf..169f1a4abb 100644 --- a/newrelic/hooks/framework_graphql.py +++ b/newrelic/hooks/framework_graphql.py @@ -47,10 +47,14 @@ def ignore_graphql_duplicate_exception(exc, val, tb): # after the original exception has already been recorded. if transaction and hasattr(val, "original_error"): - _, _, fullnames, message = parse_exc_info((None, val.original_error, None)) + while hasattr(val, "original_error"): + # Unpack lowest level original error + val = val.original_error + + _, _, fullnames, message = parse_exc_info((None, val, None)) fullname = fullnames[0] for error in transaction._errors: - if error.message == message: + if error.type == fullname and error.message == message: return True return None # Follow original exception matching rules @@ -93,6 +97,11 @@ def wrap_execute_operation(wrapped, instance, args, kwargs): except TypeError: operation = bind_operation_v2(*args, **kwargs) + if graphql_version() < (3, 0, 0): + execution_context = args[0] + else: + execution_context = instance + operation_name = get_node_value(operation, "name") trace.operation_name = operation_name = operation_name or "" @@ -106,12 +115,7 @@ def wrap_execute_operation(wrapped, instance, args, kwargs): if get_node_value(field, "name") in GRAPHQL_INTROSPECTION_FIELDS: ignore_transaction() - if graphql_version() <= (3, 0, 0): - fragments = args[ - 0 - ].fragments # In v2, args[0] is the ExecutionContext object - else: - fragments = instance.fragments # instance is the ExecutionContext object + fragments = execution_context.fragments deepest_path = traverse_deepest_unique_path(fields, fragments) trace.deepest_path = deepest_path = ".".join(deepest_path) or "" @@ -122,7 +126,9 @@ def wrap_execute_operation(wrapped, instance, args, kwargs): if deepest_path else "%s/%s" % (operation_type, operation_name) ) - transaction.set_transaction_name(transaction_name, "GraphQL", priority=14) + + if not execution_context.errors: + transaction.set_transaction_name(transaction_name, "GraphQL", priority=14) return result @@ -359,10 +365,14 @@ def wrap_resolve_field(wrapped, instance, args, kwargs): parent_type, field_asts, field_path = bind_resolve_field(*args, **kwargs) field_name = field_asts[0].name.value + field_def = parent_type.fields.get(field_name) + field_return_type = str(field_def.type) if field_def else "" with GraphQLResolverTrace(field_name) as trace: with ErrorTrace(ignore=ignore_graphql_duplicate_exception): trace._add_agent_attribute("graphql.field.parentType", parent_type.name) + trace._add_agent_attribute("graphql.field.returnType", field_return_type) + if isinstance(field_path, list): trace._add_agent_attribute("graphql.field.path", field_path[0]) else: diff --git a/tests/framework_graphql/test_application.py b/tests/framework_graphql/test_application.py index 3e5c955bd5..b5f7a13e33 100644 --- a/tests/framework_graphql/test_application.py +++ b/tests/framework_graphql/test_application.py @@ -86,7 +86,7 @@ def error_middleware(next, root, info, **args): @dt_enabled -def test_basic(app, graphql_run): +def test_basic(app, graphql_run, is_graphql_2): from graphql import __version__ as version FRAMEWORK_METRICS = [ @@ -114,6 +114,7 @@ def test_basic(app, graphql_run): "graphql.field.name": "storage_add", "graphql.field.parentType": "Mutation", "graphql.field.path": "storage_add", + "graphql.field.returnType": "[String]" if is_graphql_2 else "String", } _expected_query_operation_attributes = { "graphql.operation.type": "query", @@ -123,6 +124,7 @@ def test_basic(app, graphql_run): "graphql.field.name": "storage", "graphql.field.parentType": "Query", "graphql.field.path": "storage", + "graphql.field.returnType": "[String]", } @validate_transaction_metrics( @@ -189,7 +191,7 @@ def test_exception_in_middleware(app, graphql_run): _test_exception_rollup_metrics = [ ("Errors/all", 1), ("Errors/allOther", 1), - ("Errors/OtherTransaction/GraphQL/query/MyQuery/%s" % field, 1), + ("Errors/OtherTransaction/GraphQL/test_application:error_middleware", 1), ] + _test_exception_scoped_metrics # Attributes @@ -197,6 +199,7 @@ def test_exception_in_middleware(app, graphql_run): "graphql.field.name": field, "graphql.field.parentType": "Query", "graphql.field.path": field, + "graphql.field.returnType": "String", } _expected_exception_operation_attributes = { "graphql.operation.type": "query", @@ -205,7 +208,7 @@ def test_exception_in_middleware(app, graphql_run): } @validate_transaction_metrics( - "query/MyQuery/hello", + "test_application:error_middleware", "GraphQL", scoped_metrics=_test_exception_scoped_metrics, rollup_metrics=_test_exception_rollup_metrics + _graphql_base_rollup_metrics, @@ -224,13 +227,10 @@ def _test(): @pytest.mark.parametrize("field", ("error", "error_non_null")) @dt_enabled -def test_exception_in_resolver(app, graphql_run, is_graphql_2, field): +def test_exception_in_resolver(app, graphql_run, field): query = "query MyQuery { %s }" % field - if is_graphql_2 and field == "error_non_null": - txn_name = "_target_application:resolve_error" - else: - txn_name = "query/MyQuery/%s" % field + txn_name = "_target_application:resolve_error" # Metrics _test_exception_scoped_metrics = [ @@ -248,6 +248,7 @@ def test_exception_in_resolver(app, graphql_run, is_graphql_2, field): "graphql.field.name": field, "graphql.field.parentType": "Query", "graphql.field.path": field, + "graphql.field.returnType": "String!" if "non_null" in field else "String", } _expected_exception_operation_attributes = { "graphql.operation.type": "query", @@ -366,6 +367,7 @@ def test_field_resolver_metrics_and_attrs(app, graphql_run): "graphql.field.name": "hello", "graphql.field.parentType": "Query", "graphql.field.path": "hello", + "graphql.field.returnType": "String", } @validate_transaction_metrics( @@ -462,8 +464,13 @@ def _test(): @dt_enabled @pytest.mark.parametrize("query,expected_path", _test_queries) def test_deepest_unique_path(app, graphql_run, query, expected_path): + if expected_path == "/error": + txn_name = "_target_application:resolve_error" + else: + txn_name = "query/%s" % expected_path + @validate_transaction_metrics( - "query/%s" % expected_path, + txn_name, "GraphQL", background_task=True, ) diff --git a/tests/framework_graphql/test_application_async.py b/tests/framework_graphql/test_application_async.py index cda327d884..c49082d296 100644 --- a/tests/framework_graphql/test_application_async.py +++ b/tests/framework_graphql/test_application_async.py @@ -8,6 +8,8 @@ from testing_support.validators.validate_span_events import validate_span_events from newrelic.api.background_task import background_task +from test_application import is_graphql_2 + @pytest.fixture(scope="session") def graphql_run_async(): @@ -25,7 +27,7 @@ def graphql_run(*args, **kwargs): @dt_enabled -def test_basic_async(app, graphql_run_async): +def test_basic_async(app, graphql_run_async, is_graphql_2): from graphql import __version__ as version FRAMEWORK_METRICS = [ @@ -53,6 +55,7 @@ def test_basic_async(app, graphql_run_async): "graphql.field.name": "storage_add", "graphql.field.parentType": "Mutation", "graphql.field.path": "storage_add", + "graphql.field.returnType": "[String]" if is_graphql_2 else "String", } _expected_query_operation_attributes = { "graphql.operation.type": "query", @@ -62,6 +65,7 @@ def test_basic_async(app, graphql_run_async): "graphql.field.name": "storage", "graphql.field.parentType": "Query", "graphql.field.path": "storage", + "graphql.field.returnType": "[String]", } @validate_transaction_metrics(