Merge pull request #9 from runtastic/bugfix/database-segment-detection

Fix database segment detection

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.

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

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

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
 

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