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

[Sampler] Allow passing in a Proc to enable/disable the sampler. #134

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

bmansoob
Copy link
Contributor

@bmansoob bmansoob commented Sep 5, 2024

Small improvements for the profile sampler:

  • profile_sampler_enabled accepts a Proc now. The idea is to remove the sampler we have in Core and use this.
  • Renamed paths in the config to targets. The idea is that the sampler can be use by Jobs too -- so it makes sense to make it generic.

All changes are backwards compatible and should not break existing clients.

Copy link
Member

@dalehamel dalehamel left a comment

Choose a reason for hiding this comment

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

Awesome change, a nit about deprecation

lib/app_profiler/sampler/config.rb Outdated Show resolved Hide resolved
end

def profile_sampler_enabled
return false unless defined?(@profile_sampler_enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return false unless defined?(@profile_sampler_enabled)

You don't need that. If it's not defined then it's nil. If it's nil it isn't a Proc. So you just return nil which is close enough to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's okay, I would like to keep it as is. We enforce True/False/Proc when setting. So if we read nil and then try to set it back, it raises - which does not seem right.

@bmansoob bmansoob force-pushed the sampler-improvements branch from 8f0e196 to 1c3f41b Compare September 5, 2024 17:52
@bmansoob
Copy link
Contributor Author

bmansoob commented Sep 5, 2024

Going to squash the commits now

@bmansoob bmansoob force-pushed the sampler-improvements branch from 1c3f41b to 04a37af Compare September 5, 2024 17:58
@bmansoob
Copy link
Contributor Author

bmansoob commented Sep 6, 2024

Shipped this version to Core, everything is working as expected.

@bmansoob bmansoob merged commit dc622f3 into main Sep 6, 2024
7 checks passed
@bmansoob bmansoob deleted the sampler-improvements branch September 6, 2024 19:09
@bmansoob bmansoob mentioned this pull request Sep 6, 2024
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.

3 participants