contrib/graph-gophers/graphql-go: add graphql tracing support

[dd-caleb]

Fixes: #286

This adds GraphQL server tracing support.

[gbbr]

Let's use the tag* prefix here:

const (
	tagGraphqlField = "graphql.field"
	tagGraphqlQuery = "graphql.query"
	tagGraphqlType  = "graphql.type"
)

I find this is more Go idiomatic, we can see this in the wild too, like in the http package constants.

[gbbr]

You can leave only this line. The error will not be applied if it is nil (see here). Have you experienced otherwise?

[dd-caleb]

Yeah it's weird: https://play.golang.org/p/8JiXCGldIxi

type e struct{}

func (*e) Error() string { return "this works" }

func main() {
	var err error = (*e)(nil)
	fmt.Println(err, err == nil, err.Error())
}

It happens because an interface nil is different from a pointer nil. In this example calling the Error method actually still works though, and I guess that's why it's allowed.

I don't know if there's a better way to handle this upstream. We could do it with reflection, but that's probably not worth the computational overhead.

I can update the comment to make it more clear why its necessary if that would be helpful.

[gbbr]

I see. This could even cause us problems if people have the same expectations as me. It could result in unexpected errors where there are in fact none. There should be an issue on https://github.com/golang/go about this if there is not. I understand how this behavior is correct but can be misleading for errors. Possibly something for Go 2.

[dd-caleb]

Indeed its confusing. There's a FAQ entry and a proposal golang/go#22729

[gbbr]

Awesome! Maybe we can mention this issue in the comment:

// must explicitly check for nil, see issue golang/go#22729

[gbbr]

Based on the FAQ this looks like a beginner mistake, so I'm not sure if there is anything to do here at all, other than perhaps not make people think that it's ok to pass "nil" errors to tracer.WithError.

[gbbr]

I'm wondering if it wouldn't be better to call this NewTracer. The package is used as graphqltrace which is an action, so in the context of graphqltrace.New might indicate an action too, not a constructor. Maybe graphqltrace.NewTracer would be better and more in line with the graphql.Tracer function. Not feeling too strongly on it though.

[gbbr]

This is something outside the scope of this task, but I'm just letting you know that we have #273, which indicates that this is in some cases unexpected behavior. For example, if someone starts the tracer using:

tracer.Start(tracer.WithServiceName("my-service"))

And then later instruments a service, that should be the default service name. One solution could be to add an extra internal package at the root level to share configuration settings between the tracer package and integrations. It should be thread-safe.

[gbbr]

Would it be nicer to have tracer := graphqltrace.NewTracer()? Just so we can isolate this for the sake of the example.

[gbbr]

We could simplify this, one idea is:

switch n := len(errs); n {
    case 0:
        // err = nil
    case 1:
        err = errs[0]
    default:
        err = fmt.Errorf("%s (and %d more errors)", errs[0], n-1)
}

The fmt package formatter %s will correctly call the error interface's Error method to obtain the string.

[gbbr]

Thanks. I'm not sure I like that proposal. There's a lot to consider for sure.

…

On Tue, Jul 31, 2018 at 2:47 PM Caleb Doxsey ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In contrib/graph-gophers/graphql-go/graphql.go <#287 (comment)>: > + } + span.Finish(tracer.WithError(err)) + } +} + +// TraceField traces a GraphQL field access. +func (t *Tracer) TraceField(ctx context.Context, label string, typeName string, fieldName string, trivial bool, args map[string]interface{}) (context.Context, trace.TraceFieldFinishFunc) { + span, ctx := tracer.StartSpanFromContext(ctx, "graphql.field", + tracer.ServiceName(t.cfg.serviceName), + tracer.Tag(graphqlFieldTag, fieldName), + tracer.Tag(graphqlTypeTag, typeName), + ) + return ctx, func(err *errors.QueryError) { + // this is necessary otherwise the span gets marked as an error + if err != nil { + span.Finish(tracer.WithError(err)) Indeed its confusing. There's a FAQ entry and a proposal golang/go#22729 <golang/go#22729> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#287 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGYGlF-oTYCccUe9JXmKh8DfXsdjdsXXks5uMEPNgaJpZM4VnHTe> .

[dd-caleb]

@gbbr thanks for the review. I made the changes you requested.

For the default service name, it's a little tricky, because we set a service name by default based on the application name. I think we need some way to distinguish between a user having set the service name and us providing the default. Maybe a WithDefaultServiceName in addition to WithServiceName and the former would basically never be used by users, but only by our integrations?

Anyway that's probably best tackled in another PR since all the existing integrations will need to be updated in some way.

[gbbr]

You're right about the service name. My suggestion would also technically cause a breaking change. Let's keep it in #273

[gbbr]

Should we do the ole var _ Tracer = (*trace.Tracer)(nil) package-level constant to assert and show that this interface is implemented? Any thoughts on that? I've been using it everywhere in this package - unsure if good or bad.

[gbbr]

I forgot about the package level documentation. Can you please add that and give it a shot on godoc? It might be more intuitive to use the file graphql.go. We might want to move the canonical import path comment there too as it might be the file where people will look intuitively to find these headers.