Fix GraphQL Instrumentation

[RyanKung]

What does this pull request do?

Fix GraphQL Instrumentation

 def get_graphql_tx_name(self, graphql_doc): op = graphql_doc.definitions[0].operation

@ekampf said (#850 (comment))

There is a bug here - the first item in the definition is not necessarily an operation.
It can be a fragment in which case you'll get an exception here.

(in fact, client libraries like Apollo send fragments first and operation last so this will always fail. Its best to iterate the list and find the operation then assume its first\last)

Related issues

#850 (comment)
Fixes #966

[ghost]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: [Pull request #881 updated]

• Start Time: 2020-11-02T19:57:28.095+0000

• Duration: 26 min 57 sec

Test stats 🧪

Test	Results
Failed	0
Passed	11238
Skipped	8409
Total	19647

[beniwohli]

Just one question, other than that LGTM!

[wodCZ]

I'd propose this:

def get_graphql_tx_name(self, graphql_doc): try: op_def = [ i for i in graphql_doc.definitions if type(i).__name__ == "OperationDefinition" ][0] except KeyError: return "GraphQL unknown operation" op = op_def.operation name = op_def.name fields = op_def.selection_set.selections return "GraphQL %s %s" % (op.upper(), name if name else "+".join([f.name.value for f in fields]))

We regularly use named queries eg.:

query UnresolvedTasks { tasks (unresolved: true) { ... } } query ResolvedTasks { tasks (unresolved: false) { ... } }

Using concatenated fields instead of operation name will drop the crucial information.
I'd suggest to only use selection fields when the query is anonymous (op_def.name is falsey):

query { tasks { ... } }

Nevertheless, let's merge this as soon as possible, as anyone installing elastic-apm with graphene will currently suffer from this bug. It's also pretty hard to catch during development, as you don't usually write fragments (first) in the GraphiQL interface.

[ghost]

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	11238
Skipped	8409
Total	19647

[basepi]

I took the liberty of pushing @wodCZ's recommendation, because it was a good one. We got another user with errors from the existing graphql implementation in #966 so I want to get this merged.