Add context to validation tracing #411
Conversation
|
Open to adjusting the implementation here. As I've currently implemented it, I'm trying to avoid making this a breaking change for anyone that is actually using the validation tracer. As mentioned however, it's impossible for tracing to work properly without supplying the existing request context containing the current span, so it may be ok for this to be a 'breaking' change instead of trying to maintain backward compatibility... The |
|
@dackroyd There are merge conflicts. It seems that I have merged one of the other PRs first. Would you mind resolving these. I'm sorry for the inconvenience. |
Context is needed for tracing to access the current span, in order to add tags to it, or create child spans. As presently defined (without a context), this cannot be done: new spans could be created, but they would not be associated with the existing request trace. OpenTracingTracer implements the new interface (it never implemented the old one). Via this 'extension interface', the tracer configured (or the default tracer) will be used as the validation tracer if: * The tracer implements the (optional) interface; and * A validation tracer isn't provided using the deprecated option What this means is that the deprecated option is _preferred_ as an override. This allows users to migrate in a non-breaking, non-behaviour changing way, until such time as they intentionally remove the use of the deprecated option. For those who are currently using the default tracer, and not supplying a validation tracer, validation will be traced immediately with no change required to configuration options.
dackroyd
force-pushed
the
trace-validation-context
branch
from
d19e0c3 to
1a55b96
Compare
|
@pavelnikolov conflicts have been addressed |
| @@ -117,9 +124,10 @@ func Tracer(tracer trace.Tracer) SchemaOpt { | |||
| } | |||
|
|
|||
| // ValidationTracer is used to trace validation errors. It defaults to trace.NoopValidationTracer. | |||
| // Deprecated: context is needed to support tracing correctly. Use a Tracer which implements trace.ValidationTracerContext. | |||
There was a problem hiding this comment.
This is perfect with the comment here.
| span.SetTag("graphql.error", msg) | ||
| } | ||
| span.Finish() | ||
| } |
There was a problem hiding this comment.
Isn't this code duplicating the errors logged above? https://github.com/graph-gophers/graphql-go/pull/411/files#diff-87aab52b1f88b8febc7f0d09f51af16d5bfafb1763ddfc2e353ae47d8425de9cR36-R46
| @@ -67,6 +67,22 @@ func (OpenTracingTracer) TraceField(ctx context.Context, label, typeName, fieldN | |||
| } | |||
| } | |||
|
|
|||
| func (OpenTracingTracer) TraceValidation(ctx context.Context) TraceValidationFinishFunc { | |||
There was a problem hiding this comment.
What is calling this new method? I can't see where the method is being called.
There was a problem hiding this comment.
Nevermind, I just noticed it.
| @@ -186,7 +194,7 @@ func (s *Schema) exec(ctx context.Context, queryString string, operationName str | |||
| return &Response{Errors: []*errors.QueryError{qErr}} | |||
| } | |||
|
|
|||
| validationFinish := s.validationTracer.TraceValidation() | |||
There was a problem hiding this comment.
I wonder how was this used before without a context 🤷
Context is needed for tracing to access the current span, in order to
add tags to it, or create child spans. As presently defined (without a
context), this cannot be done: new spans could be created, but they
would not be associated with the existing request trace.
OpenTracingTracer implements the new interface (it never implemented the
old one). Via this 'extension interface', the tracer configured (or the
default tracer) will be used as the validation tracer if:
What this means is that the deprecated option is preferred as an
override. This allows users to migrate in a non-breaking, non-behaviour
changing way, until such time as they intentionally remove the use of
the deprecated option. For those who are currently using the default
tracer, and not supplying a validation tracer, validation will be traced
immediately with no change required to configuration options.