Skip to content
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 option to gqlgen contrib for skipping spans for field resolvers w/o methods #2684

Closed
samsullivan opened this issue May 3, 2024 · 5 comments · Fixed by #2695
Closed
Labels
apm:ecosystem contrib/* related feature requests or bugs enhancement quick change/addition that does not need full team approval proposal/accepted Accepted proposals

Comments

@samsullivan
Copy link
Contributor

Followup now that I've implemented #2597 and started using this middleware; also somewhat related to #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:

func (t *gqlTracer) InterceptField(ctx context.Context, next graphql.Resolver) (res any, err error) {
opCtx := graphql.GetOperationContext(ctx)
fieldCtx := graphql.GetFieldContext(ctx)

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

@samsullivan samsullivan added the enhancement quick change/addition that does not need full team approval label May 3, 2024
@github-actions github-actions bot added apm:ecosystem contrib/* related feature requests or bugs needs-triage New issues that have not yet been triaged labels May 3, 2024
devin-ai-integration bot added a commit to conorbranagan/dd-trace-go that referenced this issue May 3, 2024
@darccio
Copy link
Member

darccio commented May 9, 2024

@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 darccio removed the needs-triage New issues that have not yet been triaged label May 9, 2024
@samsullivan
Copy link
Contributor Author

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 darccio added the proposal/accepted Accepted proposals label May 14, 2024
@darccio
Copy link
Member

darccio commented May 14, 2024

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

@samsullivan
Copy link
Contributor Author

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
Copy link
Contributor Author

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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs enhancement quick change/addition that does not need full team approval proposal/accepted Accepted proposals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants