GraphQL Instrumentation #396

twcrone · 2021-08-31T18:54:19Z

Overview

GraphQL Instrumentation

Related Github Issue

For every metric created, the agent creates a scoped and unscoped version. An unscoped metric could get filtered out from being sent by the agent if it doesn't meet certain criteria. This prevents the GraphQL metrics from being filtered.

Checks

[X] Are your contributions backwards compatible with relevant frameworks and APIs?
[] Does your code contain any breaking changes? Please describe.
[X] Does your code introduce any new dependencies? Please describe.

XiXiaPdx · 2021-08-31T19:03:32Z

Manual testing of NetFlix DGS example jav app - looks good. https://staging.onenr.io/0mMRNAKLAwn

twcrone · 2021-08-31T19:03:56Z

So not really 50+ files of code. We have our GraphQL test files in separate *.gql files under src/test/resources

twcrone · 2021-08-31T20:15:57Z

We are totally fine squashing or whatever team wants to do.

jasonjkeller

A few minor nits but overall looks good!

jasonjkeller · 2021-09-01T16:56:35Z

instrumentation-test/src/main/java/com/newrelic/agent/introspec/SpanEvent.java

@@ -25,4 +27,6 @@
    String getHttpComponent();

    String getTransactionId();
+
+    Map<String, Object> getAgentAttributes();


What is the purpose of this? Looks like testing only?

Yes, so that we can get the attributes from span event through introspector. (channeling Xi Xia)

newrelic-agent/src/main/java/com/newrelic/agent/service/analytics/TracerToSpanEvent.java

...tion/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLErrorHandler.java

...entation/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLSpanUtil.java

jasonjkeller · 2021-09-01T18:56:22Z

...n/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLTransactionName.java

+        List<Selection> selection = selections.stream()
+                .filter(namedNode -> notFederatedFieldName(getNameFrom(namedNode)))
+                .collect(Collectors.toList());
+        // there can be only one, or we stop digging into query


...on/graphql-java-16.2/src/test/java/com/nr/instrumentation/graphql/GraphQLObfuscatorTest.java

meiao

Mostly nit picking.

meiao · 2021-09-01T19:32:42Z

...tion/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLErrorHandler.java

+    public static void reportResolverThrowableToNR(Throwable e) {
+        NewRelic.noticeError(e);
+    }


Do you foresee this method doing something else?
In the place that this is called, it gives the impression that it would do something else besides calling noticeError.
Same question for reportGraphQLException.

Though I do see the value in reportGraphQLError.

meiao · 2021-09-01T20:07:00Z

...entation/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLSpanUtil.java

+    private static <T> T getValueOrDefault(T value, T defaultValue) {
+        return value == null ? defaultValue : value;
+    }


This is a great method that we could use in other places.
Do we have a Util class where it could live?

Yeah, I don't know. I'm not sure where something like this goes.

meiao · 2021-09-01T20:10:28Z

...n/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLTransactionName.java

+    private static boolean isNullOrEmpty(final Collection<?> c) {
+        return c == null || c.isEmpty();
+    }


This too should be in a Util class

My rule is, if you use it twice, copy paste is fine. But if you go to use it in a third place, pull it out. But I can pull it out.

Typically I use a Collection Utils class that exists in latter versions of Java

meiao · 2021-09-01T20:27:52Z

...n/graphql-java-16.2/src/main/java/com/nr/instrumentation/graphql/GraphQLTransactionName.java

+            return null;
+        }
+        List<Selection> selections = selectionSet.getSelections();
+        if (selections == null || selections.isEmpty()) {


Util method I mentioned below could be reused here.

meiao · 2021-09-01T20:48:10Z

instrumentation/graphql-java-16.2/src/main/java/graphql/ExecutionStrategy_Instrumentation.java

+    }
+
+    protected void handleFetchingException(ExecutionContext executionContext, DataFetchingEnvironment environment, Throwable e) {
+        reportResolverThrowableToNR(e);


Following up on my comment on GraphQLErrorHandler.
I think that calling NewRelic.notifyError(e); here is less surprising for the reader without loss of the context given by the method name.

meiao · 2021-09-01T20:55:15Z

...on/graphql-java-16.2/src/test/java/com/nr/instrumentation/graphql/GraphQLObfuscatorTest.java

+
+    @ParameterizedTest
+    @CsvFileSource(resources = "/obfuscateQueryTestData/obfuscate-query-test-data.csv", delimiter = '|', numLinesToSkip = 2)
+    public void testObfuscateQuery(String queryToObfuscateFile, String expectedObfuscatedQueryFile) {


Just nit picking, but the File in the var name is odd.

meiao · 2021-09-01T21:12:15Z

instrumentation/graphql-java-16.2/build.gradle

+dependencies {
+    implementation(project(":agent-bridge"))
+
+    implementation 'com.graphql-java:graphql-java:16.2'


Do we need to add something to the THIRD_PARTY_NOTICES.md file?

??? this is the library we are instrumenting. I don't know about this type of file.

Not for dependencies used in instrumentation modules.

Typically it's just libraries that we directly use in development of the agent that need to be declared.

- Add copyright statements - Refactor out a couple utility methods - Remove a couple unnecessary passthru methods - Fix a couple unit test parameter names for clarity

twcrone and others added 30 commits August 31, 2021 08:53

Initial build.gradle to get started on GraphQL client instrumentation

b66d56e

Basic module directory for graphql work

04684fb

Base instrumentation

d086b32

Move module directory

56792b8

Add GraphQL test init

df31d74

Add Copyright comments

ea6ce43

Add genuine test of base GraphQL configuration

6bb31c4

Fix trace

67bf14f

Instrument Execution

ccc45d7

Minor tweak to test 'simple' named query transaction

8aa15bb

Add deepest unique path test

ddb44b8

Add test excluding federated field names

e3efad9

Ignore new use case for now with Fragments

7afcdb3

Add case with fragments that are not shown in name

62143b2

Add validation errors test case

f72ece9

Add test file and ignored tests for batch queries

4f0666d

enable distributed tracing for instrumentation test runner

b09c3d0

WIP trace entry point of graphql request

987c68f

WIP trace resolvers

7df53e5

WIP rename span to tx name

a6ada92

Add JUnit5 for parameterized test support

a5a3c37

Updating deps

9f27b8f

Add beginnings of parameterized tests

b163a4f

Formatted test data file

35c5c56

Move param name test files into their own directory

4deecde

Full refactored to data driven, parameterized tests

7d19a7e

Fix fragments for transaction name

97684bb

add graphql agent Attributes to span

5cbef22

wip add agent attribute to resolver for testing span attribute

186c824

capture parse, validate, and execution result errors

3923013

twcrone and others added 12 commits August 31, 2021 10:16

Checkpoint to fixing transaction names for "multiple operations"

3867bd5

Account for multiple operations in transaction name

e8758f8

set tx name right after parsing is successful

3197023

Minor refactor

eab51aa

WIP remove instrumentation and reduce spans

224e6f9

start timing in executeAsync

bc2fdc8

refactor report error instrumentation

ffca35f

obfuscate query

f7e4e9f

Fix tests based on refactoring

004e798

Remove 'post' from transaction name on parse error

5add1ee

Code cleanup

4a7cd91

Add README.md

f5ef8f9

Only support 16.x versions

cc6eae4

twcrone requested review from jasonjkeller, tbradellis and meiao August 31, 2021 21:13

jasonjkeller approved these changes Sep 1, 2021

View reviewed changes

twcrone requested a review from jasonjkeller September 1, 2021 19:42

jasonjkeller approved these changes Sep 1, 2021

View reviewed changes

meiao reviewed Sep 1, 2021

View reviewed changes

twcrone force-pushed the graphql-java branch 2 times, most recently from 495c2ea to 3ac30c1 Compare September 1, 2021 23:13

meiao approved these changes Sep 2, 2021

View reviewed changes

Cleanup from review comments

5a035c1

- Add copyright statements - Refactor out a couple utility methods - Remove a couple unnecessary passthru methods - Fix a couple unit test parameter names for clarity

twcrone force-pushed the graphql-java branch from 3ac30c1 to 5a035c1 Compare September 2, 2021 13:17

twcrone merged commit 1be967c into main Sep 2, 2021

twcrone deleted the graphql-java branch September 2, 2021 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL Instrumentation #396

GraphQL Instrumentation #396

twcrone commented Aug 31, 2021

XiXiaPdx commented Aug 31, 2021

twcrone commented Aug 31, 2021

twcrone commented Aug 31, 2021

jasonjkeller left a comment

jasonjkeller Sep 1, 2021

twcrone Sep 1, 2021

jasonjkeller Sep 1, 2021

meiao left a comment

meiao Sep 1, 2021

twcrone Sep 1, 2021

meiao Sep 1, 2021

twcrone Sep 1, 2021

meiao Sep 1, 2021

twcrone Sep 1, 2021

twcrone Sep 1, 2021

meiao Sep 1, 2021

meiao Sep 1, 2021

meiao Sep 1, 2021

meiao Sep 1, 2021

twcrone Sep 1, 2021

jasonjkeller Sep 1, 2021

jasonjkeller Sep 1, 2021

GraphQL Instrumentation #396

GraphQL Instrumentation #396

Conversation

twcrone commented Aug 31, 2021

Overview

Related Github Issue

Checks

XiXiaPdx commented Aug 31, 2021

twcrone commented Aug 31, 2021

twcrone commented Aug 31, 2021

jasonjkeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meiao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment