Skip first runtime report when eager load false #233

kbaum · 2019-04-23T14:48:13Z

What you uncovered here, gave me an idea for skipping first coverage report in rails when eager load is false. Let me know what you think.

danmayer · 2019-04-23T15:21:17Z

hmm this is interesting, I don't think it would resolve the resque issue, I would have to undo my other fix to test it out... I think the problem/difference is that coverage.save_report is called multiple times as part of initialization we try to call when rails loads, in resque by the time the first real Coverage call should get called it was really the 3rd call. I think there is a bug/issue related to that, but I guess this changes the behavior to explicitly skip once vs skip until there is a valid previous result...

I can run some tests on it, thinking we need some more tests around real world start ups with different settings between processes and threads.

danmayer · 2019-04-23T15:22:52Z

Did you see I avoided skipping any results on the resque fix case here: https://github.com/danmayer/coverband/pull/232/files#diff-8d2f5459991b3a79d0afa782146382f3R6

danmayer · 2019-04-23T15:24:05Z

I think this is a good direction I am wondering if we ever want to skip results or if it is a first hit we send to :eager_loading vs :runtime The skip was kind of a hack for data ending up in the wrong type...

kbaum · 2019-04-23T15:36:00Z

My thought was now that you have discovered that rails rake tasks skip eager load, we can pivot our logic based on whether eager load is set.

Re: The resque fix. We could change that to call #skip_next_report!. This makes our intentions a little more explicit.

I am wondering if we ever want to skip results or if it is a first hit we send to :eager_loading vs :runtime

I'm not 100% sure on this. I don't think we can send to eager_load because there doesn't seem to be a way for us to know when a class is being loaded when eager_load is not set since classes are loaded on demand. Even skipping first coverage report is not a guarantee as a class can be loaded at any time although I feel like skipping the first n reports may be the best solution.

danmayer · 2019-04-23T15:46:13Z

My thought was now that you have discovered that rails rake tasks skip eager load, we can pivot our logic based on whether eager load is set.

So the issues we were having heroku and runtime coverage, I don't think were eager loading, as it was eager loading.

was the rake asset calls (fixed by skipping), although this eager check since rake disables might resolve that as well.
eager_load was set but previous data wasn't there on heroku

Re: The resque fix. We could change that to call #skip_next_report!. This makes our intentions a little more explicit.

in this case wouldn't it be a call to #don't_skip_next_report! as the current code is setting empty results to avoid the first hit getting skipped?

I am wondering if we ever want to skip results or if it is a first hit we send to :eager_loading vs :runtime

I'm not 100% sure on this. I don't think we can send to eager_load because there doesn't seem to be a way for us to know when a class is being loaded when eager_load is not set since classes are loaded on demand. Even skipping first coverage report is not a guarantee as a class can be loaded at any time although I feel like skipping the first n reports may be the best solution.

yeah I guess if we explicitly document and call out it isn't loading it is eager_loading and kind of Rails specific... This feature definitely has had more edge cases and make me wonder how things outside of Rails are looking... Sinatra, Rack, etc... Perhaps if not Rails it should just have one metric as we previously did?

danmayer · 2019-04-23T18:10:48Z

OK, the RC3 release is live on high volume sites and with a couple of hours don't seem to have any miscategorized traffic.

I think I might test this PR and some of the other ideas around load vs runtime coverage for the next release. thoughts? @kbaum

kbaum · 2019-04-23T22:27:20Z

Agreed on this being a next release thing.

Think eager loading coverage could be applicable outside of rails. Non rails projects like gems tend to require most files on startup. I think it can still be helpful to know what usage came on load of files on process start vs at runtime. I think this is true for most Sinatra projects as well though I am not sure. Without this separation of load vs runtime, I imagine it would be difficult to figure out which gems are unused within any bundler file regardless of whether it’s a rails project.

danmayer · 2019-04-23T22:49:27Z

yeah I agree it is very useful just trying to figure out if it is as easily detectable... I don't know if various other frameworks have config.eager_loading flags or if there are standard ways to defer to frameworks / apps to load files.

I can try to search around and see what other common plug-in like Ruby systems do... If they have patterns for other Rack / CLI based tool chains

kbaum · 2019-04-23T23:10:03Z

I don't know if various other frameworks have config.eager_loading flags or if there are standard ways to defer to frameworks / apps to load files.

Not sure either but projects can do this manually with coverband. For example:

Bundler.require(:coverband_group)
require 'coverband'
Coverband.eager_loading_coverage!
Bundler.require(:default)
require 'thing1'
require 'thing2'

require 'sinatra'

get '/' do
  "Hello, world. Here is the content of HELLO_URL (#{uri}): #{response.body.inspect}\n"
end

Coverband.runtime_coverage!

kbaum · 2019-04-23T23:11:51Z

lib/coverband/integrations/resque.rb

@@ -4,7 +4,7 @@
  Coverband.start
  Coverband.runtime_coverage!
  # no reason to miss coverage on a first resque job
-  Coverband::Collectors::Delta.set_default_results
+  Coverband.coverage.disable_skip_next_report!


@danmayer - what do you think of something like this?

nice much more understandable, big +1

danmayer · 2019-04-24T00:21:16Z

True that is a great simple example perhaps we just document that really well and have the app log some helpful messages if a user is doing something that doesn't match expectations... like if they never call loading or send it data but start sending runtime data... We could alert, "Coverband is working fine, but we weren't able to detect code loading so all coverage is considered run-time"

kbaum · 2019-04-24T01:55:47Z

I like the idea of the warning.

kbaum · 2019-04-24T12:21:23Z

test/integration/full_stack_test.rb

@@ -22,8 +21,7 @@ def setup
    Coverband.configuration.store.clear!
    Coverband.start
    Coverband::Collectors::Coverage.instance.runtime!
-    @rack_file = File.expand_path(TEST_RACK_APP, File.dirname(__FILE__))
-    require @rack_file
+    @rack_file = require_unique_file 'fake_app/basic_rack.rb'


@danmayer - was running into sporadic errors. Making use of require_unique_file seems to clear things up.

danmayer · 2019-04-27T02:28:59Z

I will try to test this again some apps and confirm it is working how we like so we can get it merged soon

…_eager_load" This reverts commit 1c44e03, reversing changes made to 7acfcbc.

kbaum · 2019-04-28T10:53:30Z

Note: This PR was merged then reverted. We ended up going with #238.

kbaum force-pushed the set_skip_runtime_hit_on_eager_load branch from 0dbcc9f to 563c1ef Compare April 23, 2019 14:52

kbaum requested a review from danmayer April 23, 2019 14:52

kbaum added 2 commits April 23, 2019 18:54

Skip first runtime report when eager load false

06af7a4

Add disable skip next report

9cefffd

kbaum force-pushed the set_skip_runtime_hit_on_eager_load branch from 563c1ef to 9cefffd Compare April 23, 2019 23:03

kbaum commented Apr 23, 2019

View reviewed changes

Make rack file require unique

78a8f63

kbaum commented Apr 24, 2019

View reviewed changes

Add test for runtime/eager_loading coverage

959b52a

kbaum mentioned this pull request Apr 27, 2019

Prepend eager load #238

Merged

danmayer merged commit 1c44e03 into master Apr 27, 2019

danmayer deleted the set_skip_runtime_hit_on_eager_load branch April 27, 2019 21:38

danmayer pushed a commit that referenced this pull request Apr 27, 2019

Revert "Merge pull request #233 from danmayer/set_skip_runtime_hit_on…

82b8859

…_eager_load" This reverts commit 1c44e03, reversing changes made to 7acfcbc.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip first runtime report when eager load false #233

Skip first runtime report when eager load false #233

kbaum commented Apr 23, 2019 •

edited

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

kbaum Apr 23, 2019

danmayer Apr 24, 2019

danmayer commented Apr 24, 2019

kbaum commented Apr 24, 2019

kbaum Apr 24, 2019

danmayer commented Apr 27, 2019

kbaum commented Apr 28, 2019

Skip first runtime report when eager load false #233

Skip first runtime report when eager load false #233

Conversation

kbaum commented Apr 23, 2019 • edited

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

danmayer commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

danmayer commented Apr 23, 2019

kbaum commented Apr 23, 2019

kbaum Apr 23, 2019

Choose a reason for hiding this comment

danmayer Apr 24, 2019

Choose a reason for hiding this comment

danmayer commented Apr 24, 2019

kbaum commented Apr 24, 2019

kbaum Apr 24, 2019

Choose a reason for hiding this comment

danmayer commented Apr 27, 2019

kbaum commented Apr 28, 2019

kbaum commented Apr 23, 2019 •

edited