feat: Suppress internal spans by default #1533
Conversation
…metry-ruby-contrib into update-suppress-config
jterapin
requested review from
fbogsany,
mwear,
robertlaurin,
dazuma,
ericmustin,
arielvalentin,
ahayworth,
plantfansam,
robbkidd,
simi,
kaylareopelle,
xuan-cao-swi and
a team
as code owners
|
Was this or the related Issue discussed at a SIG? seems like i'm missing some context, at the very least I have some ambivalence about multiple config options that do the same thing. |
I have not attended SIG so most likely not. This PR drops the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
|
👋 This pull request has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the |
| MessagingHelper.inject_context_if_supported(context, client_method, service_id) | ||
|
|
||
| if HandlerHelper.instrumentation_config[:suppress_internal_instrumentation] | ||
| if HandlerHelper.skip_internal_instrumentation? |
There was a problem hiding this comment.
@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.
|
|
||
| private | ||
|
|
||
| def resolve_config(config) |
There was a problem hiding this comment.
How are we providing backward compatability for the deprecated option?
Is it not supposed to work at all?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull Request Overview
This PR inverts the default behavior for internal span instrumentation in the AWS SDK, suppressing internal spans by default to avoid unexpected costs for users.
- Deprecated
:suppress_internal_instrumentationconfig option - Added new
:enable_internal_instrumentationconfig (defaults to false) - Updated tests to verify the new behavior and remove assertions for internal spans that are now suppressed by default
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation.rb | Added new config option, deprecated old one, and added deprecation warning |
| handler_helper.rb | Added helper method to determine if internal instrumentation should be skipped |
| handler.rb | Updated to use new helper method and removed trailing whitespace |
| telemetry.rb | Updated to use new helper method for determining span suppression |
| README.md | Updated documentation to reflect new config options and defaults |
| telemetry_test.rb | Refactored tests to focus on client spans and added specific test for internal spans when enabled |
| instrumentation_test.rb | Added tests for default config and deprecated config handling |
| handler_test.rb | Updated require path and code formatting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| c.use 'OpenTelemetry::Instrumentation::AwsSdk', { | ||
| inject_messaging_context: true, | ||
| suppress_internal_instrumentation: true | ||
| enable_internal_instrumentation: true |
There was a problem hiding this comment.
Use spaces instead of tabs for indentation to maintain consistency with the rest of the file.
| enable_internal_instrumentation: true | |
| enable_internal_instrumentation: true |
Copilot uses AI. Check for mistakes.
| # This configuration has been deprecated in a favor of `:enable_internal_instrumentation` | ||
| # | ||
| # @example An explicit default configuration | ||
| # @example An explicit default configurations |
There was a problem hiding this comment.
Corrected 'configurations' to 'configuration' for grammatical consistency.
| # @example An explicit default configurations | |
| # @example An explicit default configuration |
Copilot uses AI. Check for mistakes.
#1502
Description: The
aws_sdkinstrumentation now suppresses internal spans by default. This ensures that the additional low-level instrumentation does not generate additional costs for customers unknowingly.The following work has been done to accommodate:
:suppress_internal_instrumentationconfig:enable_internal_instrumentationwhich is set false by defaultTo enable internal spans, users will have explicitly configure as follows: