Skip to content

feat!: Suppress internal spans by default #1533

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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Oct 11, 2025
Merged

feat!: Suppress internal spans by default #1533

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
59ae02a
Suppress internal spans by default
jterapin May 13, 2025
7185ad2
Merge branch 'main' into update-suppress-config
jterapin May 13, 2025
d9b44ce
Update specs
jterapin May 13, 2025
c8df9ed
Merge branch 'update-suppress-config' of github.com:jterapin/opentele…
jterapin May 13, 2025
07d0a02
Remove unnecessary gems
jterapin May 13, 2025
f8f0694
Merge branch 'main' into update-suppress-config
jterapin May 13, 2025
7ec9182
Merge branch 'main' into update-suppress-config
jterapin Aug 30, 2025
29f377e
Address feedback
jterapin Aug 30, 2025
a2b71e8
Improve deprecated config impl
jterapin Aug 30, 2025
e7e5045
Update to just include warn
jterapin Aug 30, 2025
502b4da
Merge branch 'main' into update-suppress-config
arielvalentin Oct 11, 2025
9eb4e9b
Merge branch 'main' into update-suppress-config
arielvalentin Oct 11, 2025
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
8 changes: 5 additions & 3 deletions instrumentation/aws_sdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

Use spaces instead of tabs for indentation to maintain consistency with the rest of the file.

Suggested change
enable_internal_instrumentation: true
enable_internal_instrumentation: true

Copilot uses AI. Check for mistakes.
}
end
```
Expand All @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor

@arielvalentin arielvalentin Aug 31, 2025

Choose a reason for hiding this comment

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

@kaylareopelle I realize this is a common pattern we have in the code base already but I would recommend we look into refactoring cases like this where we create separate mixins for "untraced" instead of evaluating this logic with every request.

OpenTelemetry::Common::Utilities.untraced { super }
else
super
Expand All @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/handler_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 20 additions & 7 deletions instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Copilot AI Oct 4, 2025

Choose a reason for hiding this comment

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

Corrected 'configurations' to 'configuration' for grammatical consistency.

Suggested change
# @example An explicit default configurations
# @example An explicit default configuration

Copilot uses AI. Check for mistakes.
# 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)
Expand All @@ -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']
Expand All @@ -65,6 +69,15 @@ def gem_version

private

def resolve_config(config)
Copy link
Contributor

@arielvalentin arielvalentin Aug 31, 2025

Choose a reason for hiding this comment

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

How are we providing backward compatability for the deprecated option?

Is it not supposed to work at all?

Copy link
Contributor Author

@jterapin jterapin Sep 8, 2025

Choose a reason for hiding this comment

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

Well - this configuration handling was more trickier than i thought.....

The main challenge is that configuration hashes always contain default values, which makes it difficult to identify which settings were explicitly set by users.

I tried to solve this by removing the :suppress_internal_instrumentation option completely to detect user config, but discovered that unrecognized settings aren't passed through to the install block.

Then, I realized that users would only want to touch this setting to suppress internal traces- which happens to be the default behavior anyway. Any thoughts on mitigating this?

Copy link
Contributor

@arielvalentin arielvalentin Sep 9, 2025

Choose a reason for hiding this comment

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

Then I suppose it can be a breaking change there. We would need to call out in the release notes that the option is no longer available.

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'
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/aws_sdk/lib/opentelemetry/instrumentation/aws_sdk/patches/telemetry.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 4 additions & 7 deletions instrumentation/aws_sdk/test/opentelemetry/handler_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
14 changes: 14 additions & 0 deletions instrumentation/aws_sdk/test/opentelemetry/instrumentation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Loading