Skip to content

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Jun 28, 2022

Graphql Integration

Overview

This integration supports graphql-core 3 and graphql-core 2. It provides instrumentation for executing graphql queries using graphql.graphql(), graphql.graphql_sync(), graphql.execute(), and graphql.subscribe(). This integration also wraps the execution of graphql.parse() and graphql.validate() in a span.

Supports

All libraries which use graphql-core>=2.0 to execute queries are supported:

  • graphene>=2.0
  • strawberry>=0.16.0
  • ariadne>=0.9.0

Implementation Details

The graphql-core integration provides support for parsing, building, validating, and executing graphql schemas. It also provides additional insight into resolving GraphQLFields. This integration uses bytecode instrumentation to add tracing to graphql operations listed below.

When graphql.graphql() or graphql.graphql_sync (in graphql-core>=3) is called the following trace is produced:

  • span name: graphql.query, resource: graphql source string
    • span name: graphql.parse, resource: graphql.parse
      converts the body of an incoming graphql request into a graphql document
    • span name: graphql.validate, resource: graphql.validate
      ensures a graphql schema is syntactically correct and can be executed in a given context
    • span name: graphql.execute, resource: graphql source string
      executes a valid graphql schema and returns a response
      • span name: graphql.resolve, resource: name of graphql field
      • span name: graphql.resolve, resource: name of graphql field
      • ........ (a span is produced for every resolved field)
        Graphql queries can be complex and often resolve multiple Graphql fields in single request. Instrumenting resolvers provides insight on the execution of individual graphql fields and can highlight bottlenecks in fetching specific types of data. However for some applications this can produce large traces.

Note: If a GraphqlError is raised while parsing or validating a graphql query then graphql.execute will not be called and instrumented. For this reason we add the graphql source string to the graphql.query span AND the graphql.execute span.

Disabling graphql.resolve

Graphql queries can be complex and can resolve a large number of graphql fields in one request. Many of the resolvers could provide little to no insight into the execution of the request (ex. if the default resolver is called). To simplify the flamegraph produced by the graphql integration users can set DD_TRACE_GRAPHQL_PATCH_RESOLVERS=False to disable resolver instrumentation.

Future Work

Add Span Tags

Add span tags to maintain feature parity with the .NET and javascript tracer.

ddtrace-js graphql integration: https://github.com/DataDog/dd-trace-js/tree/c9f82be2b256b2894d894e2eb49c331852b1262b/packages/datadog-plugin-graphql/src

Instrument Promises

In graphql-core>=2,<3, graphql.execute() can return a promise if the graphql.execute() is called with return_promise=True. This promise will execute the graphql query asynchronously. To fix this we need to start the graphql.execute span when the promise is fulfilled and NOT when the promise is created.
POC: 52ebd46

Refactor patching

#3878 (comment)

Testing strategy

Validate that the traces in the graphql snapshot tests produce meaningful data.
Ensure patch, execute, validate, and query operation are patched everywhere they are imported. Ex. graphql.execute(), graphql.execution.execute(), graphql.execution.execute.execute() or graphql.graphql.execute() should produce the same instrumentation.

Relevant issue(s)

Resolves #925.

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • PR cannot be broken up into smaller PRs.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added for fixes and features, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.
  • Add to milestone.

@mabdinur mabdinur marked this pull request as ready for review June 28, 2022 18:32
@mabdinur mabdinur requested a review from a team as a code owner June 28, 2022 18:32
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from 6426cf6 to 70b9fab Compare June 28, 2022 18:46
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from e31212a to 245664d Compare July 1, 2022 14:46
ZStriker19
ZStriker19 previously approved these changes Jul 1, 2022
Copy link
Collaborator

@ZStriker19 ZStriker19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job and thank you for all of the comments and the in-depth description!

@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 245664d to b19bac4 Compare July 6, 2022 21:05
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from f2cfd3c to b19bac4 Compare July 6, 2022 22:04
P403n1x87
P403n1x87 previously approved these changes Jul 7, 2022
Copy link
Contributor

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯
Just an aside: this change has both a declarative and an imperative flavour. If the idea of using the internal wrapping module really flies, perhaps all the imperative boilerplate could be factored out in some higher-level abstraction around wrap/unwrap and leave just the declarative part in integration sources.

@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 1e8dea9 to e70fe4a Compare July 11, 2022 15:18
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from 799b309 to 06e60f8 Compare July 11, 2022 16:48
@mabdinur mabdinur requested review from P403n1x87 and ZStriker19 July 11, 2022 16:56
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 06e60f8 to b65d9c1 Compare July 11, 2022 17:42
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from b65d9c1 to 341b184 Compare July 11, 2022 17:59
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from 9c9b0e3 to dab208e Compare July 12, 2022 18:06
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Mostly nits and one openish question to the team:

@DataDog/apm-python should this integration be named "graphql" or "graphql-core". The package/pypi name for this integration is "graphql-core" but it is used by most/all graphql libraries.

@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 31c7cb5 to 1d4b48b Compare July 28, 2022 13:57
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 1d4b48b to 26a50e4 Compare July 28, 2022 14:26
@mabdinur mabdinur requested a review from Kyle-Verhoog July 28, 2022 17:27
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 26a50e4 to 3964841 Compare July 28, 2022 18:08
@mabdinur mabdinur requested a review from Kyle-Verhoog July 28, 2022 19:09
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Jul 28, 2022
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Jul 28, 2022
@Kyle-Verhoog Kyle-Verhoog added this to the v1.4.0 milestone Jul 29, 2022
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch 2 times, most recently from 1388e8e to 0d65d09 Compare July 29, 2022 13:48
mabdinur and others added 2 commits July 29, 2022 09:59
Co-authored-by: Kyle Verhoog <[email protected]>

Update ddtrace/contrib/graphql/__init__.py

Co-authored-by: Kyle Verhoog <[email protected]>

Update ddtrace/contrib/graphql/patch.py

Co-authored-by: Brett Langdon <[email protected]>

Update ddtrace/contrib/graphql/patch.py

Co-authored-by: Brett Langdon <[email protected]>

Update ddtrace/contrib/graphql/patch.py

Co-authored-by: Brett Langdon <[email protected]>
@mabdinur mabdinur force-pushed the graphql-support-with-middleware branch from 0d65d09 to 272829e Compare July 29, 2022 14:00
@codecov-commenter
Copy link

Codecov Report

Merging #3878 (d32db26) into 1.x (396d0bc) will increase coverage by 0.11%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##              1.x    #3878      +/-   ##
==========================================
+ Coverage   79.48%   79.59%   +0.11%     
==========================================
  Files         714      717       +3     
  Lines       55787    56199     +412     
==========================================
+ Hits        44344    44734     +390     
- Misses      11443    11465      +22     
Impacted Files Coverage Δ
ddtrace/_monkey.py 82.00% <ø> (ø)
ddtrace/contrib/graphql/patch.py 86.33% <86.33%> (ø)
ddtrace/contrib/django/utils.py 90.27% <87.50%> (-0.27%) ⬇️
tests/contrib/graphql/test_graphql.py 99.00% <99.00%> (ø)
ddtrace/appsec/processor.py 92.59% <100.00%> (+0.18%) ⬆️
ddtrace/contrib/graphql/__init__.py 100.00% <100.00%> (ø)
ddtrace/contrib/jinja2/patch.py 92.64% <100.00%> (+0.10%) ⬆️
ddtrace/contrib/pylons/middleware.py 85.33% <100.00%> (+0.82%) ⬆️
ddtrace/contrib/trace_utils.py 94.59% <100.00%> (+0.03%) ⬆️
ddtrace/ext/__init__.py 100.00% <100.00%> (ø)
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@mergify mergify bot merged commit e618c0f into 1.x Jul 29, 2022
@mergify mergify bot deleted the graphql-support-with-middleware branch July 29, 2022 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add graphql support
6 participants