dd-trace-go icon indicating copy to clipboard operation
dd-trace-go copied to clipboard

Add option to gqlgen contrib for skipping spans for field resolvers w/o methods

Open samsullivan opened this issue 9 months ago • 5 comments

Followup now that I've implemented https://github.com/DataDog/dd-trace-go/issues/2597 and started using this middleware; also somewhat related to https://github.com/DataDog/dd-trace-go/issues/2610 in that we're both aiming to reduce overly verbose traces from the same contrib package.

Problem: the contrib package for gqlgen creates separate spans for each field being resolved, even if it can automatically be inferred by the fields on the struct (instead of requiring a separate function).

Consider the following GQL type:

type Book {
  author: Author!
  title: String!
}

Consider it being mapped to the following Golang type:

type Book struct {
  AuthorID int
  Title    string
}

In this case, you would need to implement a function to resolve the book's author via the author ID; this is in contrast to the title, which gqlgen can determine automatically.

Solution: another option, such as WithSkipFieldsWithoutMethods, that would skip creating a span downstream of the InterceptField method here: https://github.com/DataDog/dd-trace-go/blob/3a426cad6b642bc0dab36d49c02f8e09b40529e5/contrib/99designs/gqlgen/tracer.go#L139-L141

It would be checked with fieldCtx.IsMethod; I'm happy to contribute and would be able to do it alongside #2610, also.

samsullivan avatar May 03 '24 05:05 samsullivan

@samsullivan Thanks for reaching out! We'll take a look (cc @rarguelloF @zarirhamza). I'm a bit surprised about the fix created by Devin. Was it guided by you or did it create the PR completely autonomously?

darccio avatar May 09 '24 08:05 darccio

Interesting, I didn't even notice the Devin AI actions until your comment. I am not involved and would be happy to handwrite something similar w/tests and w/o AI, if it sounds like a reasonable addition...

samsullivan avatar May 09 '24 12:05 samsullivan

@samsullivan It sounds like a reasonable addition. We'll be glad to review it once it's ready.

darccio avatar May 14 '24 14:05 darccio

I'm having a hard time testing this since I don't see a good way of forcing gqlgen's graphql.Fieldcontext type to have IsMethod = true with the middleware test library's current implementation of gqlgen in newTestClient.

samsullivan avatar May 14 '24 23:05 samsullivan

Nevermind; took a fresh look this morning and dug into the gqlgen test server to figure out how to accurately test this case. Should be ready to review whenever, thanks! :+1:

samsullivan avatar May 15 '24 17:05 samsullivan