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.

Sign up for GitHub

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

Jump to bottom

contrib/99designs/gqlgen: additional options to reduce noise #2695

Merged
merged 21 commits into from
Jun 27, 2024
Merged

contrib/99designs/gqlgen: additional options to reduce noise #2695

merged 21 commits into from
Jun 27, 2024

Conversation

samsullivan
Copy link
Contributor

@samsullivan samsullivan commented May 15, 2024 •
edited
Loading

What does this PR do?

Adds two new configuration options to contrib/99designs/gqlgen, which have a similar end goal of reducing noise:

  • SkipFieldsForIntrospectionQuery
    • if the OperationName is IntrospectionQuery, spans are not created for each field
  • SkipFieldsWithTrivialResolver
    • if the field resolver is trivial (doesn't require a function / can be accessed directly from the parent object), spans are not created

Motivation

We started using contrib/99designs/gqlgen middleware recently and noticed it creates way more spans than we intended, most of which have little value for our needs.

Fixes #2684. Fixes #2610.

Reviewer's Checklist

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

@samsullivan samsullivan requested review from a team as code owners May 15, 2024 04:08
samsullivan
samsullivan commented May 15, 2024
View reviewed changes
contrib/99designs/gqlgen/tracer.go
Comment on lines -162 to +175
Trivial: !(fieldCtx.IsMethod || fieldCtx.IsResolver), // TODO: Is this accurate?
Trivial: isTrivial,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Is this accurate?

Seems like it to me:

@samsullivan samsullivan mentioned this pull request May 15, 2024
Closed
@samsullivan
Copy link
Contributor Author

Alternate naming: WithOmitTrivialFields and WithOmitIntrospectionQuery 🤷

@seh
Copy link

seh commented May 27, 2024

Alternate naming: WithOmitTrivialFields and WithOmitIntrospectionQuery 🤷

How about WithTrivialFieldsOmitted and WithIntrospectionQueriesOmitted?

What else remains to settle in order to merge this patch?

@samsullivan
Copy link
Contributor Author

Alternate naming: WithOmitTrivialFields and WithOmitIntrospectionQuery 🤷

How about WithTrivialFieldsOmitted and WithIntrospectionQueriesOmitted?

Can push tomorrow.

What else remains to settle in order to merge this patch?

Nothing but review, as far as I'm aware.

@samsullivan
Copy link
Contributor Author

@darccio I was just signing on for the day; did you have any opinion on the naming?

@darccio
Copy link
Member

darccio commented May 28, 2024

@darccio I was just signing on for the day; did you have any opinion on the naming?

Being honest, I would drop the With from both options, but I decided to keep them as you all seemed comfortable with them 😁 If you meant between Skip and Omit I prefer Skip at the beginning of the name, so the user can be sure about what it does (and Skip sounds more clear to me than Omit).

@samsullivan
Copy link
Contributor Author

@darccio I was just signing on for the day; did you have any opinion on the naming?

Being honest, I would drop the With from both options, but I decided to keep them as you all seemed comfortable with them 😁 If you meant between Skip and Omit I prefer Skip at the beginning of the name, so the user can be sure about what it does (and Skip sounds more clear to me than Omit).

Agreed, but had simply been trying to follow other patterns existing elsewhere in the repo; pushed to shorten these by removing With prefix (fns start w/Skip).

rarguelloF
rarguelloF requested changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@rarguelloF rarguelloF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! Just a small change to keep the existing options naming convention.

contrib/99designs/gqlgen/option.go Outdated Show resolved Hide resolved
@samsullivan
Copy link
Contributor Author

Thanks for the review @rarguelloF; should be good to go now 👍

@rarguelloF rarguelloF enabled auto-merge (squash) June 27, 2024 09:21
@rarguelloF rarguelloF merged commit 5fcda9f into DataDog:main Jun 27, 2024
71 checks passed
@samsullivan samsullivan deleted the gqlgen/options-to-reduce-noise branch June 30, 2024 03:35
@samsullivan
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darccio darccio darccio approved these changes

@rarguelloF rarguelloF rarguelloF approved these changes

Assignees
No one assigned
Labels
waiting-for-release
Projects
None yet
Milestone
No milestone
4 participants
@samsullivan @seh @darccio @rarguelloF