search

DataDog / dd-trace-go

Datadog Go Library including APM tracing, profiling, and security monitoring.
https://docs.datadoghq.com/tracing/
Other
651 stars 435 forks source link

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

Closed samsullivan closed 2 months ago

samsullivan commented 4 months ago

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.

darccio commented 4 months ago

@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?

samsullivan commented 4 months ago

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...

darccio commented 4 months ago

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

samsullivan commented 4 months ago

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 commented 4 months ago

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: