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

prevent false 'unsatisfied' dependency status #2914

Merged
merged 2 commits into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions lib/new_relic/dependency_detection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,9 @@ def defer(&block)

def detect!
@items.each do |item|
if item.dependencies_satisfied?
item.execute
else
item.configure_as_unsatisfied unless item.disabled_configured?
end
next if item.executed || item.disabled_configured?

item.dependencies_satisfied? ? item.execute : item.configure_as_unsatisfied
end
end

Expand Down Expand Up @@ -65,6 +63,13 @@ def dependencies_satisfied?
end

def configure_as_unsatisfied
# TODO: currently using :unsatisfied for Padrino will clobber the value
# already set for Sinatra, so skip Padrino and circle back with a
# new Padrino specific solution in the future.
#
# https://github.com/newrelic/newrelic-ruby-agent/issues/2912
Copy link
Contributor

Choose a reason for hiding this comment

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

Really appreciate the comment here about what's going on with this. thank you!

return if name == :padrino

NewRelic::Agent.config.instance_variable_get(:@cache)[config_key] = :unsatisfied
end

Expand Down
7 changes: 7 additions & 0 deletions test/multiverse/suites/sinatra/sinatra_test_cases.rb
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ def test_with_regex_pattern
assert_metrics_recorded(["Controller/Sinatra/#{app_name}/#{regex_segment}"])
end

def test_that_the_actively_configured_instrumentation_is_not_marked_as_unsatsfied
get('/pass')

assert_equal 200, last_response.status
assert_includes(%w[chain prepend], NewRelic::Agent.config[:'instrumentation.sinatra'].to_s)
end

# https://support.newrelic.com/tickets/31061
def test_precondition_not_over_called
get('/precondition')
Expand Down
17 changes: 17 additions & 0 deletions test/new_relic/dependency_detection_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -488,4 +488,21 @@ def test_prepend_is_replaced_by_unsatisfied_when_appropriate
assert_equal :unsatisfied, dd.config_value
end
end

def test_already_executed_items_are_not_executed_again
unexecuted = Minitest::Mock.new
unexecuted.expect :executed, false
unexecuted.expect :dependencies_satisfied?, true
unexecuted.expect :disabled_configured?, false
unexecuted.expect :execute, -> { execution_took_place = true }
executed = Minitest::Mock.new
executed.expect :executed, true
unexecuted.expect :disabled_configured?, false

DependencyDetection.instance_variable_set(:@items, [unexecuted, executed])
DependencyDetection.detect!

unexecuted.verify
executed.verify
end
end
Loading