feat!: Suppress internal spans by default (open-telemetry#1533)

* Suppress internal spans by default

* Update specs

* Remove unnecessary gems

* Address feedback

* Improve deprecated config impl

* Update to just include warn

---------

Co-authored-by: Ariel Valentin <[email protected]>

Author: 2 people

instrumentation/aws_sdk/README.md

@@ -20,7 +20,7 @@ To install the instrumentation, call `use` with the name of the instrumentation.
 OpenTelemetry::SDK.configure do |c|
   c.use 'OpenTelemetry::Instrumentation::AwsSdk', {
     inject_messaging_context: true,
-    suppress_internal_instrumentation: true
+	enable_internal_instrumentation: true
   }
 end
 ```
@@ -36,8 +36,10 @@ end
 This instrumentation offers the following configuration options: 
 * `:inject_messaging_context` (default: `false`): When set to `true`, adds context key/value 
  to Message Attributes for SQS/SNS messages.
-* `suppress_internal_instrumentation` (default: `false`): When set to `true`, any spans with 
- span kind of `internal` are suppressed from traces.
+* `:enable_internal_instrumentation` (default: `false`): When set to `true`, any spans with 
+ span kind of `internal` are traced.
+* `:suppress_internal_instrumentation`: **Deprecated**. This configuration has been 
+ deprecated in a favor of `:enable_internal_instrumentation`
 
 ## Integration with SDK V3's Telemetry support
 AWS SDK for Ruby V3 added support for Observability which includes a new configuration, 

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb

@@ -23,7 +23,7 @@ def call(context)
           ) do |span|
             MessagingHelper.inject_context_if_supported(context, client_method, service_id)
 
-            if HandlerHelper.instrumentation_config[:suppress_internal_instrumentation]
+            if HandlerHelper.skip_internal_instrumentation?
               OpenTelemetry::Common::Utilities.untraced { super }
             else
               super
@@ -32,7 +32,6 @@ def call(context)
                 OpenTelemetry::SemanticConventions::Trace::HTTP_STATUS_CODE,
                 context.http_response.status_code
               )
-
               if (err = response.error)
                 span.record_exception(err)
                 span.status = Trace::Status.error(err.to_s)

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler_helper.rb

@@ -14,6 +14,10 @@ def instrumentation_config
             AwsSdk::Instrumentation.instance.config
           end
 
+          def skip_internal_instrumentation?
+            instrumentation_config[:enable_internal_instrumentation] == false
+          end
+
           def client_method(service_id, context)
             "#{service_id}.#{context.operation.name}".delete(' ')
           end

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/instrumentation.rb

@@ -19,24 +19,27 @@ module AwsSdk
       # - `false` **(default)** - Context key/value will not be added.
       # - `true` - Context key/value will be added.
       #
-      # ### `:suppress_internal_instrumentation`
+      # ### `:enable_internal_instrumentation`
+      # Enables tracing of spans of `internal` span kind.
       #
-      # Disables tracing of spans of `internal` span kind.
+      # - `false` **(default)** - Internal spans are not traced
+      # - `true` - Internal spans are traced.
       #
-      # - `false` **(default)** - Internal spans are traced.
-      # - `true` - Internal spans are not traced.
+      # ### `:suppress_internal_instrumentation` (deprecated)
+      # This configuration has been deprecated in a favor of `:enable_internal_instrumentation`
       #
-      # @example An explicit default configuration
+      # @example An explicit default configurations
       #   OpenTelemetry::SDK.configure do |c|
       #     c.use 'OpenTelemetry::Instrumentation::AwsSdk', {
       #       inject_messaging_context: false,
-      #       suppress_internal_instrumentation: false
+      #       enable_internal_instrumentation: false
       #     }
       #   end
       class Instrumentation < OpenTelemetry::Instrumentation::Base
         MINIMUM_VERSION = Gem::Version.new('2.0.0')
 
-        install do |_config|
+        install do |config|
+          resolve_config(config)
           require_dependencies
           patch_telemetry_plugin if telemetry_plugin?
           add_plugins(Seahorse::Client::Base, *loaded_service_clients)
@@ -52,6 +55,7 @@ class Instrumentation < OpenTelemetry::Instrumentation::Base
 
         option :inject_messaging_context, default: false, validate: :boolean
         option :suppress_internal_instrumentation, default: false, validate: :boolean
+        option :enable_internal_instrumentation, default: false, validate: :boolean
 
         def gem_version
           if Gem.loaded_specs['aws-sdk']
@@ -65,6 +69,15 @@ def gem_version
 
         private
 
+        def resolve_config(config)
+          return unless config[:suppress_internal_instrumentation]
+
+          OpenTelemetry.logger.warn(
+            'Instrumentation AwsSdk configuration option suppress_internal_instrumentation has been deprecated,' \
+            'use enable_internal_instrumentation option instead'
+          )
+        end
+
         def require_dependencies
           require_relative 'handler'
           require_relative 'handler_helper'

instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/patches/telemetry.rb

@@ -26,7 +26,7 @@ def span_wrapper(context, &)
             ) do |span|
               MessagingHelper.inject_context_if_supported(context, client_method, service_id)
 
-              if HandlerHelper.instrumentation_config[:suppress_internal_instrumentation]
+              if HandlerHelper.skip_internal_instrumentation?
                 OpenTelemetry::Common::Utilities.untraced { super }
               else
                 yield span

instrumentation/aws_sdk/test/opentelemetry/handler_test.rb

@@ -4,7 +4,7 @@
 #
 # SPDX-License-Identifier: Apache-2.0
 
-require 'test_helper'
+require_relative '../test_helper'
 
 describe OpenTelemetry::Instrumentation::AwsSdk do
   describe 'AwsSdk Plugin' do
@@ -95,8 +95,7 @@
         client.publish(message: 'msg', phone_number: '123456')
 
         _(span.name).must_equal('phone_number publish')
-        _(span.attributes[otel_semantic::MESSAGING_DESTINATION])
-          .must_equal('phone_number')
+        _(span.attributes[otel_semantic::MESSAGING_DESTINATION]).must_equal('phone_number')
       end
     end
 
@@ -178,8 +177,7 @@
 
           client.get_queue_url(queue_name: 'queue-name')
 
-          _(span.attributes['messaging.destination'])
-            .must_equal('unknown')
+          _(span.attributes['messaging.destination']).must_equal('unknown')
           _(span.attributes).wont_include('messaging.url')
         end
       end
@@ -193,8 +191,7 @@
 
         client.list_tables
 
-        _(span.attributes[otel_semantic::DB_SYSTEM])
-          .must_equal('dynamodb')
+        _(span.attributes[otel_semantic::DB_SYSTEM]).must_equal('dynamodb')
       end
     end
   end

instrumentation/aws_sdk/test/opentelemetry/instrumentation_test.rb

@@ -65,4 +65,18 @@
       instrumentation.instance_variable_set(:@installed, false)
     end
   end
+
+  it 'with default options' do
+    instrumentation.instance_variable_set(:@installed, false)
+    instrumentation.install
+    _(instrumentation.config[:inject_messaging_context]).must_equal(false)
+    _(instrumentation.config[:enable_internal_instrumentation]).must_equal(false)
+    _(instrumentation.config[:suppress_internal_instrumentation]).must_equal(false)
+  end
+
+  it 'honors deprecated config, :suppress_internal_instrumentation' do
+    instrumentation.instance_variable_set(:@installed, false)
+    instrumentation.install(suppress_internal_instrumentation: true)
+    _(instrumentation.config[:enable_internal_instrumentation]).must_equal(false)
+  end
 end