contrib/99designs/gqlgen/tracer.go: nil check response

[dienvoandpadcojp]

What does this PR do?

Check result from graphql responseHandler is nil related to issue https://github.com/DataDog/dd-trace-go/issues/2791

Motivation

I think I need contribute to community

Reviewer's Checklist

• [x] Changed code has unit tests for its functionality at or near 100% coverage.
• [ ] System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
• [ ] There is a benchmark for any new code, or changes to existing code.
• [ ] If this interacts with the agent in a new way, a system test has been added.
• [ ] Add an appropriate team label so this PR gets put in the right place for the release notes.
• [ ] Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

[darccio]

@dienvoandpadcojp Thank you for contributing this. Would it possible for you to add a test proving that your fix does what is expected? Thanks!

[dienvoandpadcojp]

hi @darccio , I've added test proving that my fix doesn't cause panic

[dienvoandpadcojp]

Hi @darccio , may I ask about this PR. Is there any changes are needed on my part? because I don't have the appropriate permissions to merge this PR to main and I don't know what I need to do next

[darccio]

Hi @dienvoandpadcojp. I meant to come back with our feedback, but I had other issues to handle. Unfortunately, your PR raises some questions:

• Why the majority of the function is in a defer function?
• It seems that the real change is just one line: if response != nil && len(response.Errors) > 0 {. Wouldn't be enough to just add the response != nil to the condition?

About the review process for external contributions, we do a gardening session weekly. We didn't have the opportunity to review this PR until this week meeting. In that meeting IDM (the team who has the last word on contribs in this repository) asked to reconsider my approval.

Please, can you address the two previous questions?

[dienvoandpadcojp]

Thanks for your comment, let's me explain the changes

Why the majority of the function is in a defer function?

I saw the previous change version the query.Finish(...) and req.Finish(...) always be executed after the responseHandler was executed. Beside, there was already had defer function when finishing span (if it not nil). So I thought it should be in the defer function.

It seems that the real change is just one line: if response != nil && len(response.Errors) > 0 {. Wouldn't be enough to just add the response != nil to the condition?

You're right, I added logic check response != nil before the previous version which already check len(response.Errors) > 0 . I've check the logic of tracer.WithError and it ok to set err arg by nil. I will update the PR

[github-actions[bot]]

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.