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

feat(usage): at-least-once and dynamic sampling #26

Merged
merged 71 commits into from
Aug 19, 2024

Conversation

charcoalyy
Copy link
Contributor

@charcoalyy charcoalyy commented Aug 6, 2024

Context

GraphQL Hive Client has an option for at least once sampling that is not included in the ruby gem. As a side, this option also allows dynamic sampling via passing in a function.

Currently, graphql-ruby-hive only allows for a fixed sampling rate. With this update, you can input either a fixed sampling rate or a dynamic sampler. You can also choose to sample all distinct operations at least once. See new README.md for specific configuration details.

Changes

  • BasicSampler created for fixed sampling rates, with the option to sample all operations at least once
  • DynamicSampler created for dynamic sampling, with the option to sample all operations at least once
  • Sampler created to manage sampling logic for UsageReporter to use
  • passing a sampling rate to collect_usage_sampling still works, but will also log a deprecated field warning

Testing

Unit tests have been included for all Sampler classes and updated for UsageReporter.

This gem was tested locally with the following cases:

  • sample rate (both deprecated and updated version)
  • dynamic sampler
  • sample rate with at least once sampling
  • dynamic sampler with at least once sampling
  • sample rate with at least once sampling and custom key generator
  • dynamic sampler with at least once sampling and custom key generator

Cases were consistent with behaviour documented in README.
sampler test cases

@charcoalyy charcoalyy changed the title (WIP) at least one sampler (WIP) at-least-one dynamic sampling Aug 7, 2024
@charcoalyy charcoalyy changed the title (WIP) at-least-one dynamic sampling feat(usage): at-least-one dynamic sampling Aug 12, 2024
@charcoalyy charcoalyy changed the title feat(usage): at-least-one dynamic sampling feat(usage): at-least-one and dynamic sampling Aug 12, 2024
@charcoalyy charcoalyy changed the title feat(usage): at-least-one and dynamic sampling feat(usage): at-least-once and dynamic sampling Aug 12, 2024
@charcoalyy charcoalyy marked this pull request as ready for review August 12, 2024 21:06
Copy link
Collaborator

@aryascripts aryascripts left a comment

Choose a reason for hiding this comment

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

Leaving as a comment because I don't disagree with any of the code written here strongly, but I do have some suggestions about clarifying the config arguments better.

And, some discussion on log levels if we have wrong configs. Just wondering about others' thoughts on this as well!

lib/graphql-hive/usage_reporter.rb Outdated Show resolved Hide resolved
lib/graphql-hive/sampler/basic_sampler.rb Outdated Show resolved Hide resolved
lib/graphql-hive/sampler/sampling_context.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@aryascripts aryascripts left a comment

Choose a reason for hiding this comment

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

This is great! Ty for all the fixes after comments.

README.md Show resolved Hide resolved
Copy link
Owner

@rperryng rperryng left a comment

Choose a reason for hiding this comment

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

Looks great!

My only thought was that if we're introducing a "new" API for configuration that we should try and match the official NodeJS client as closely as possible, but I see this would conflict with some other feedback related to some values being multiple types (e.g. boolean and function) - which I agree is maybe a bit confusing as I don't think this is as common in ruby code (probably due to lack of strong type-safety). So looks good to me.

@rperryng rperryng merged commit f02c141 into rperryng:master Aug 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants