Skip to content

Conversation

@alxgsv
Copy link

@alxgsv alxgsv commented May 29, 2013

Useful in cases like:

profile.profile_if(Rails.env.staging?, ...) { ... }

profile.profile_if(current_user.admin?, ...) { ... }

@lmarburger
Copy link
Collaborator

I like the idea but I'm not too fond of the API. Since this is Ruby, you'll want a #profile_unless method. This would also be useful with #profile_files_matching which means #profile_files_matching_if and #profile_files_matching_unless methods.

Do you have some code where this change makes your life easier? Pilfer::Middleware does something similar. It's effectively:

def run
  if should_profile?
    profiler.profile(&method(:call_downstream))
  else
    call_downstream
  end
end

@alxgsv
Copy link
Author

alxgsv commented May 29, 2013

Yes, I use it (that's why I made it). If you don't like it — okay, it's just idea that helps me :-)

It's true about unless and profile_files_matching_if. I think I just want some global flag that indicates that profiling should be turned on. With that we can have it conditionally turned on if before_filters.

@danthompson
Copy link

Just my two cents, but I don't care for the API either. 😕

I suppose my major concern would be with the duplication of language-provided constructs as methods. Also, if you have criteria that determines if and when profiling runs, I believe that shouldn't be the profilers concern.

Perhaps, wrapping the profiler with your logic and conditions would be a clearer approach like @lmarburger illustrates.

@eric
Copy link
Owner

eric commented Jun 1, 2013

The vision that I've had is that the user of pilfer would have an entirely different class that would be responsible for deciding if a given call should be profiled or not.

Something that I think would be really cool is if it were possible to signal to a given process to profile on demand, but I don't think that sort of logic makes sense to be in pilfer itself right now.

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.

4 participants