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

AutoInstruments - Automatically break down uninstrumented time #247

Merged
merged 47 commits into from Sep 9, 2019

Conversation

dlanderson
Copy link
Contributor

@dlanderson dlanderson commented Feb 20, 2019

AutoInstruments breaks down uninstrumented Controller time automatically, by parsing the AST tree of your own application code when it boots.

Early adopters should try out the AutoInstruments Beta in staging and development environments.

You can try out this Beta by following these steps.

If you have any questions please reach out to support@scoutapp.com!

@jsierles
Copy link

Note that this branch currently requires disabling bootsnap.

@ioquatix
Copy link
Contributor

What was wrong with the Rails module usage? Just curious.

@dlanderson
Copy link
Contributor Author

What was wrong with the Rails module usage? Just curious.

@jsierles and others hit an exception with newer Rail versions: uninitialized constant ActiveSupport::Executor (NameError). It seemed to be caused by a Rails loader while Rails was in the process of being loaded itself. Avoiding it entirely seems the safest bet.

@ioquatix
Copy link
Contributor

I have rebased on master.

@jonekdahl
Copy link

Is there already a config switch to enable/disable AutoInstruments? Otherwise, I'd like to request one.

@ioquatix
Copy link
Contributor

@jonekdahl

if ENV['AUTO_INSTRUMENT']
  require 'scout_apm/auto_instrument'
end

Does that work for you?

@jonekdahl
Copy link

@ioquatix Yes, that seems to work, thanks. Although I think people will expect a regular Scout config, like http://help.apm.scoutapp.com/#profile.

@jsierles
Copy link

Has this work been abandoned? We tried this on our production environment, and found that many traces showed even less information the previous traces, so we reverted.

@ioquatix
Copy link
Contributor

@jsierles do you mind giving me a bit more feedback?

We could try rebasing on master. In theory it shouldn't be less than before, and ideally more.

@jeremy
Copy link

jeremy commented Jun 24, 2019

# Added AFTER `File.expand_path('../../Gemfile', __FILE__)`
# But BEFORE `require 'bundler/setup'`
require 'scout_apm/auto_instrument'

How can this work, considering scout_apm is itself bundled?

@ioquatix
Copy link
Contributor

@jeremy I believe this goes into your own config config/boot.rb so File.expand_path('../../Gemfile', __FILE__) resolves to Gemfile in your app - is that what you were questioning?

@jeremy
Copy link

jeremy commented Jun 27, 2019

@ioquatix Bundler setup is what sets up the load path so bundled gems can be required… so how can we require scout_apm BEFORE Bundler setup, unless you require installation as a system gem? (Pardon if I'm missing something here!)

@ioquatix
Copy link
Contributor

@itsderek23 if you want to do a rebase on master I am also happy with that.

@itsderek23
Copy link
Contributor

@ioquatix - thanks! Not ready quite yet - we're getting our ducks in a row for this.

@ioquatix
Copy link
Contributor

I would suggest hard reset to 4e8a98a and rebase on master, then squash merge. There are lots of WIP commits. Let me know if you want me to squash it down.

@ioquatix
Copy link
Contributor

I am making the requested change to the class name tracking and squashing this branch in preparation for merging.

@itsderek23
Copy link
Contributor

@ioquatix - naming update is looking good. Makes it far more intuitive to go to the source in the UI. Thanks!

image

@ioquatix
Copy link
Contributor

That's awesome!

@itsderek23
Copy link
Contributor

@ioquatix - what do you recommend we call the code pieces we are instrumenting? It's more fine-grained than lines.

For example:

if A || B looks to be broken up into instrumenting A and B separately.

@ioquatix
Copy link
Contributor

Expressions?

Method invocations?

@ioquatix ioquatix force-pushed the auto_instruments branch 2 times, most recently from 53823de to 61c6a05 Compare August 13, 2019 21:38
@itsderek23
Copy link
Contributor

Verified autoinstruments now works without any changes to config/boot.rb. Thanks @ioquatix!

@ioquatix
Copy link
Contributor

That’s awesome!

itsderek23 and others added 4 commits August 27, 2019 06:52
This allows toggling of the autoinstruments install via a `auto_instrument` config option (default=true).

AutoInstruments are not installed (regardless of the `auto_instrument` config value) if not using a supported Ruby version.

To read the config file, I needed to move the loading of auto instruments in the Railstie after the agent is installed. I'm still seeing the same output after moving this after agent install.
Toggle install of autoinstruments via a config setting
@jonekdahl
Copy link

Is there any chance of this getting merged and included in a release, keeping it disabled by default? It would allow us to use the same version of the gem in all environments and enable the feature in selected test environments. Using a different version/branch of a gem in a specific environment in a continuous delivery pipeline is a pain.

@itsderek23
Copy link
Contributor

@jonekdahl,

Is there any chance of this getting merged and included in a release, keeping it disabled by default?

This is our plan. We want to get some more installs of the auto_instruments branch before merging into master, but we don't anticipate this lasting more than a month.

@ioquatix
Copy link
Contributor

ioquatix commented Sep 3, 2019

@dlanderson I've added some initial checks for local assignments. I've added some tests, but I still need to see the code which caused #281 to fail.

@ioquatix ioquatix closed this Sep 3, 2019
@ioquatix ioquatix reopened this Sep 3, 2019
@ioquatix
Copy link
Contributor

ioquatix commented Sep 3, 2019

Sorry, hit wrong button.

@itsderek23
Copy link
Contributor

@jonekdahl - We've merged this into master (version 2.6.0).

To enable autoinstruments, set the auto_instruments config option to true.

Details in our docs.

@mathieujobin
Copy link
Contributor

@itsderek23 docs still suggest we should update our config/boot.rb
from this PR conversation, it sounds like this is outdated

@dlanderson
Copy link
Contributor Author

@mathieujobin Apologies - docs need updating. We have released AutoInstruments in 2.6.0. To enable, just set auto_instruments: true in scout_apm.yml, or SCOUT_AUTO_INSTRUMENTS=true in Env vars if Heroku.

@itsderek23
Copy link
Contributor

itsderek23 commented Sep 19, 2019

@mathieujobin - Apologies. I forgot to publish the changes.

Docs are updated.

/cc @dlanderson

@mathieujobin
Copy link
Contributor

getting a syntax error on deploy

I, [2019-09-19T23:26:12.525601 #20423]  INFO -- : Refreshing Gem list
warning: parser/current is loading parser/ruby24, which recognizes
warning: 2.4.7-compliant syntax, but you are running 2.4.6.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
/home/rails/.rvm/gems/ruby-2.4.6@payroll_hero/gems/scout_apm-2.6.0/lib/scout_apm/auto_instrument/instruction_sequence.rb:11:in `compile': members_controller.rb:32: syntax error, unexpected tOP_ASGN, expecting keyword_end (SyntaxError)
roy'"]){department.members} -= [::ScoutApm::AutoInstrument("
                              ^
        from /home/rails/.rvm/gems/ruby-2.4.6@payroll_hero/gems/scout_apm-2.6.0/lib/scout_apm/auto_instrument/instruction_sequence.rb:11:in `load_iseq'
        from /home/rails/.rvm/gems/ruby-2.4.6@payroll_hero/gems/activesupport-4.2.11.1/lib/active_support/dependencies.rb:274:in `require'

@ioquatix
Copy link
Contributor

I can take a look

@mathieujobin
Copy link
Contributor

this is in our code

    def destroy
      department.members -= [current_account.employees.find_by!(account_specific_id: params.require(:id))]

      respond_to do |format|
        format.html {}

        format.json do
          render json: members_as_json
        end
      end
    end

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.

None yet

7 participants