Add new GraphQL tracer complying to span attributes specification #3672
Conversation
vpellan
force-pushed
the
vpellan/new-graphql-tracing
branch
from
142aec9
to
e22728b
Compare
There was a problem hiding this comment.
I read this PR and it looks really good, discussed it with @vpellan and I had two suggestions:
- https://github.com/DataDog/dd-trace-rb/blob/master/docs/GettingStarted.md appears to be the source for https://docs.datadoghq.com/tracing/trace_collection/automatic_instrumentation/dd_libraries/ruby/, add the new
with_unified_traceroption into the options table there - For the graphql methods overridden by the tracer, add catch-all arguments
*args, **kwargsto permit graphql library to add additional arguments to the methods which currently (with the PR as it is right now) would not be possible because this PR effectively freezes method signatures.
| module Tracing | ||
| module Contrib | ||
| module GraphQL | ||
| # These methods will be called by the GraphQL runtime to trace the execution of queries |
There was a problem hiding this comment.
Can you add a short blurb here about what exactly the UnifiedTrace represents, specially if compared with the default TracePatcher?
I want to make sure we have enough context in the future to know whether we should switch to use this module by default, in the next major version. (actually, if it is desirable to switch to the UnifiedTrace by default, let's write that down in the comments here. We used the keyword # DEV-3.0: when looking into possible changes for the next major version upgrade, so we can do the same here, if it applies)
| span.set_tag('graphql.operation.type', query.selected_operation.operation_type) | ||
| span.set_tag('graphql.operation.name', query.selected_operation_name) if query.selected_operation_name | ||
| query.provided_variables.each do |key, value| | ||
| span.set_tag("graphql.variables.#{key}", value) |
There was a problem hiding this comment.
Do we have a concern with unbound carnality here? In case query.provided_variables is a very large list?
There was a problem hiding this comment.
There is two possibilities : First option, we use provided_variables, which takes every variable even the ones that are not used in the query. This could indeed create concern for unbound cardinality. Second option: We use variables.storage. This is a map of the variables that will be used in the query, thus reducing concerns about unbound cardinality, but with less information sent in the trace.
| end | ||
|
|
||
| def execute_field_span(callable, span_key, **kwargs) | ||
| platform_key = @platform_key_cache[UnifiedTrace].platform_field_key_cache[kwargs[:field]] |
There was a problem hiding this comment.
@platform_key_cache doesn't seem to be initialized anywhere. Could you clarify where it comes from? (and probably document it in code, unless it's super trivial and I just personally missed it)
There was a problem hiding this comment.
It is initialized upstream, in ::GraphQL::Tracing::PlatformTrace. I will add a comment to clarify it.
| # Implement this method in a subclass to apply custom tags to datadog spans | ||
| # @param key [String] The event being traced | ||
| # @param data [Hash] The runtime data for this event (@see GraphQL::Tracing for keys for each event) | ||
| # @param span [Datadog::Tracing::SpanOperation] The datadog span for this event | ||
| # def prepare_span(key, data, span) | ||
| # end |
There was a problem hiding this comment.
Is the prepare_span method commented out for a particular reason? Or did we just forget to uncomment it?
There was a problem hiding this comment.
The reason is to show how you can add custom tags to your spans. We can delete these comments and add it to the documentation instead.
…documentation for prepare_span
What does this PR do?
This PR adds a new GraphQL tracer that follows Datadog's span attributes specification. This can be activated with the option "with_unified_tracer: true". This new tracer also create a span for every field instead of only the non-scalar ones.
Motivation:
Following the span attributes spec is required to use API Catalog
Additional Notes: