From 7d22a9f40c1a2f10e3411462dde464e9d2d6a288 Mon Sep 17 00:00:00 2001 From: Dominik Goltermann Date: Thu, 11 Oct 2018 16:35:24 +0200 Subject: [PATCH] Fix database segment detection --- Changelog.md | 6 ++- lib/new_relic/manticore/instrumentation.rb | 13 +++-- lib/newrelic/manticore/version.rb | 2 +- .../manticore/instrumentation_test.rb | 47 ++++++++++++------- test/test_helper.rb | 13 +++++ 5 files changed, 56 insertions(+), 25 deletions(-) create mode 100644 test/test_helper.rb diff --git a/Changelog.md b/Changelog.md index ffc5f94..1efc00a 100644 --- a/Changelog.md +++ b/Changelog.md @@ -4,7 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.1.1] - 2018-10-11 +## [0.1.2] - 2018-10-11 +### Fixed +Detection whether manticore is used inside a database segment was not reliable and is now more robust + +## [0.1.1] - 2018-10-09 ### Fixed - Time spent inside Manticore requests inside database calls where not included in the exclusive time stats of the database request. - Fixed instrumentation when using manticore with faraday adapter. diff --git a/lib/new_relic/manticore/instrumentation.rb b/lib/new_relic/manticore/instrumentation.rb index c2bfd43..0ba959f 100644 --- a/lib/new_relic/manticore/instrumentation.rb +++ b/lib/new_relic/manticore/instrumentation.rb @@ -19,14 +19,14 @@ module Manticore # operation def self.create_segment? state = NewRelic::Agent::TransactionState.tl_get - return false unless state && state.current_transaction + return false unless state && + state.current_transaction - existing_segments = state.current_transaction.segments + return true unless state.current_transaction.current_segment - existing_segments.empty? || - !existing_segments.last.is_a?( - ::NewRelic::Agent::Transaction::DatastoreSegment - ) + !state.current_transaction.current_segment.is_a?( + ::NewRelic::Agent::Transaction::DatastoreSegment + ) end # rubocop:disable Metrics/BlockLength @@ -81,7 +81,6 @@ def execute_with_newrelic_trace! def call_with_newrelic_trace if NewRelic::Manticore.create_segment? segment = create_newrelic_segment - segment.add_request_headers(WrappedRequest.new(@request)) on_complete do |response| begin diff --git a/lib/newrelic/manticore/version.rb b/lib/newrelic/manticore/version.rb index 83fcdfe..0356e51 100644 --- a/lib/newrelic/manticore/version.rb +++ b/lib/newrelic/manticore/version.rb @@ -2,6 +2,6 @@ module Newrelic module Manticore - VERSION = "0.1.1".freeze + VERSION = "0.1.2".freeze end end diff --git a/test/new_relic/manticore/instrumentation_test.rb b/test/new_relic/manticore/instrumentation_test.rb index 03763f9..f3c61ef 100644 --- a/test/new_relic/manticore/instrumentation_test.rb +++ b/test/new_relic/manticore/instrumentation_test.rb @@ -1,16 +1,6 @@ # frozen_string_literal: true -$LOAD_PATH.unshift File.expand_path("../lib", __dir__) - -require "minitest/autorun" -require "minitest/unit" -require "rspec/mocks/minitest_integration" -require "newrelic_rpm" -require "faraday" -require "faraday/adapter/manticore" -require "manticore" -require "new_relic/manticore" -require "pry" +require "./test/test_helper" NewRelic::Agent.require_test_helper DependencyDetection.detect! @@ -72,7 +62,8 @@ class InstrumentationTest < Minitest::Test end end - assert_metrics_recorded("Datastore/operation/Elasticsearch/Search" => { call_count: 1 }) + metric = "Datastore/operation/Elasticsearch/Search" + assert_metrics_recorded(metric => { call_count: 1 }) end end @@ -101,16 +92,40 @@ class InstrumentationTest < Minitest::Test end describe "when manticore is used as transport for a database (e.g. in Elasticsearch)" do - it "does not create an external request segment" do - expect(NewRelic::Agent::External).not_to receive(:start_segment) + it "does not deduct manticore time from exclusive database time" do + client = ::Manticore::Client.new + expect(client.client).to receive(:execute).and_wrap_original do |original, *args| + sleep(1) + original.call(*args) + end in_transaction do NewRelic::Agent::Datastores.wrap("Elasticsearch", "Search", "index_name") do - ::Manticore.post(request_uri, body: "data").body + client.post(request_uri, body: "data").body end end - assert_metrics_recorded("Datastore/operation/Elasticsearch/Search" => { call_count: 1 }) + db_metric = "Datastore/operation/Elasticsearch/Search" + database_spec = metric_spec_from_specish(db_metric) + exclusive_time = NewRelic::Agent.instance.stats_engine.to_h[database_spec].total_exclusive_time + + assert_operator(exclusive_time, :>, 1.0) + + assert_metrics_recorded(db_metric => { call_count: 1 }) + assert_metrics_not_recorded("External/#{external_service}/Manticore/POST" => { call_count: 1 }) + end + + describe "when the last segment before the manticore segment is a finished database segment" do + it "does create an external request segment" do + in_transaction do + NewRelic::Agent::Datastores.wrap("Elasticsearch", "Search", "index_name") do + sleep(0.01) + end + ::Manticore.post(request_uri, body: "data").body + end + + assert_metrics_recorded("External/#{external_service}/Manticore/POST" => { call_count: 1 }) + end end end diff --git a/test/test_helper.rb b/test/test_helper.rb new file mode 100644 index 0000000..cea376f --- /dev/null +++ b/test/test_helper.rb @@ -0,0 +1,13 @@ +$LOAD_PATH.unshift File.expand_path("../lib", __dir__) + +require "minitest/autorun" +require "minitest/unit" +require "rspec/mocks/minitest_integration" +require "pry" +require "newrelic_rpm" +require "faraday" +require "faraday/adapter/manticore" +require "manticore" +require "new_relic/manticore" + +RSpec::Mocks.configuration.verify_partial_doubles = true