-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add context to validation tracing #411
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,16 +25,23 @@ import ( | |
// resolver, then the schema can not be executed, but it may be inspected (e.g. with ToJSON). | ||
func ParseSchema(schemaString string, resolver interface{}, opts ...SchemaOpt) (*Schema, error) { | ||
s := &Schema{ | ||
schema: schema.New(), | ||
maxParallelism: 10, | ||
tracer: trace.OpenTracingTracer{}, | ||
validationTracer: trace.NoopValidationTracer{}, | ||
logger: &log.DefaultLogger{}, | ||
schema: schema.New(), | ||
maxParallelism: 10, | ||
tracer: trace.OpenTracingTracer{}, | ||
logger: &log.DefaultLogger{}, | ||
} | ||
for _, opt := range opts { | ||
opt(s) | ||
} | ||
|
||
if s.validationTracer == nil { | ||
if tracer, ok := s.tracer.(trace.ValidationTracerContext); ok { | ||
s.validationTracer = tracer | ||
} else { | ||
s.validationTracer = &validationBridgingTracer{tracer: trace.NoopValidationTracer{}} | ||
} | ||
} | ||
|
||
if err := s.schema.Parse(schemaString, s.useStringDescriptions); err != nil { | ||
return nil, err | ||
} | ||
|
@@ -68,7 +75,7 @@ type Schema struct { | |
maxDepth int | ||
maxParallelism int | ||
tracer trace.Tracer | ||
validationTracer trace.ValidationTracer | ||
validationTracer trace.ValidationTracerContext | ||
logger log.Logger | ||
useStringDescriptions bool | ||
disableIntrospection bool | ||
|
@@ -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. | ||
func ValidationTracer(tracer trace.ValidationTracer) SchemaOpt { | ||
return func(s *Schema) { | ||
s.validationTracer = tracer | ||
s.validationTracer = &validationBridgingTracer{tracer: tracer} | ||
} | ||
} | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder how was this used before without a context 🤷 |
||
validationFinish := s.validationTracer.TraceValidation(ctx) | ||
errs := validation.Validate(s.schema, doc, variables, s.maxDepth) | ||
validationFinish(errs) | ||
if len(errs) != 0 { | ||
|
@@ -272,6 +280,14 @@ func (s *Schema) validateSchema() error { | |
return nil | ||
} | ||
|
||
type validationBridgingTracer struct { | ||
tracer trace.ValidationTracer | ||
} | ||
|
||
func (t *validationBridgingTracer) TraceValidation(context.Context) trace.TraceValidationFinishFunc { | ||
return t.tracer.TraceValidation() | ||
} | ||
|
||
func validateRootOp(s *schema.Schema, name string, mandatory bool) error { | ||
t, ok := s.EntryPoints[name] | ||
if !ok { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is calling this new method? I can't see where the method is being called. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, I just noticed it. |
||
span, _ := opentracing.StartSpanFromContext(ctx, "Validate Query") | ||
|
||
return func(errs []*errors.QueryError) { | ||
if len(errs) > 0 { | ||
msg := errs[0].Error() | ||
if len(errs) > 1 { | ||
msg += fmt.Sprintf(" (and %d more errors)", len(errs)-1) | ||
} | ||
ext.Error.Set(span, true) | ||
span.SetTag("graphql.error", msg) | ||
} | ||
span.Finish() | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this code duplicating the errors logged above? https://github.com/graph-gophers/graphql-go/pull/411/files#diff-87aab52b1f88b8febc7f0d09f51af16d5bfafb1763ddfc2e353ae47d8425de9cR36-R46 |
||
} | ||
|
||
func noop(*errors.QueryError) {} | ||
|
||
type NoopTracer struct{} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,25 @@ | ||
package trace | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/graph-gophers/graphql-go/errors" | ||
) | ||
|
||
type TraceValidationFinishFunc = TraceQueryFinishFunc | ||
|
||
// Deprecated: use ValidationTracerContext. | ||
type ValidationTracer interface { | ||
TraceValidation() TraceValidationFinishFunc | ||
} | ||
|
||
type ValidationTracerContext interface { | ||
TraceValidation(ctx context.Context) TraceValidationFinishFunc | ||
} | ||
|
||
type NoopValidationTracer struct{} | ||
|
||
// Deprecated: use a Tracer which implements ValidationTracerContext. | ||
func (NoopValidationTracer) TraceValidation() TraceValidationFinishFunc { | ||
return func(errs []*errors.QueryError) {} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfect with the comment here.