Skip to content
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.

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

Add SNS/SQS trace propagation #3842

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add SNS/SQS trace propagation #3842

wants to merge 1 commit into from

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Aug 12, 2024

This PR propagates tracing through AWS SNS and SQS messages.

This is disabled by default, but can enabled with one of these options:

# Through an environment variable...
DD_TRACE_AWS_PROPAGATION_ENABLED=true

# .. or programmatically
Datadog.configure do |c|
  c.tracing.instrument :aws, propagation: true
end

It's also possible to configure if the local span will become the child of a distributed parent span (with DD_TRACE_AWS_TRACE_PARENTAGE_STYLE=distributed) or if it will keep the local span parentage as usual (DD_TRACE_AWS_TRACE_PARENTAGE_STYLE=local). The default is distributed. But because the overall feature is disabled by default (DD_TRACE_AWS_PROPAGATION_ENABLED is false by default), span parentage is not affected out of the box.

Additional Notes:

This is the first step to support distributed observability across queues in APM Ruby. There is future work scheduled to support SNS and SQS batch workflows, as well as adding the same observability to the Kafka integration.

How to test the change?
All changes have new tests added.

Unsure? Have a question? Request a review!


data = {}
if Datadog::Tracing::Contrib.inject(trace.to_digest, data)
message_attributes['_datadog'] = { :data_type => data_type, :binary_value => data.to_json }

Choose a reason for hiding this comment

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

Code Quality Violation

Use newer syntax for a hash with symbols as keys (...read more)

The rule "Use new syntax when keys are symbols" is a coding standard in modern Ruby development. It encourages the use of the new hash syntax, introduced in Ruby 1.9, where symbols are used as keys. The old hash rocket syntax (:symbol => value) is replaced with the more elegant and succinct symbol: value syntax.

This rule is important as it promotes a cleaner, more readable code. The new syntax is more concise and less cluttered, making it easier to understand the structure and purpose of the hash. This is particularly beneficial in large codebases or when hashes are nested or complex.

To adhere to this rule, always use the new syntax when defining hashes where keys are symbols. Instead of defining a hash with :symbol => value, use symbol: value. This approach will not only make your code more readable but also ensure consistency across your codebase.

View in Datadog  Leave us feedback  Documentation

def extract_propagation!(response, data_type)
messages = response.data.messages

return unless (message = messages[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
return unless (message = messages[0])
return unless (message = messages.first)
Improve readability with first (...read more)

This rule encourages the use of first and last methods over array indexing to access the first and last elements of an array, respectively. The primary reason behind this rule is to improve code readability. Using first and last makes it immediately clear that you are accessing the first or last element of the array, which might not be immediately obvious with array indexing, especially for developers who are new to Ruby.

The use of these methods also helps to make your code more idiomatic, which is a crucial aspect of writing effective Ruby code. Idiomatic code is easier to read, understand, and maintain. It also tends to be more efficient, as idioms often reflect patterns that are optimized for the language.

To adhere to this rule, replace the use of array indexing with first or last methods when you want to access the first and last elements of an array. For instance, instead of arr[0] use arr.first and instead of arr[-1] use arr.last. However, note that this rule should be applied only when reading values. When modifying the first or last elements, array indexing should still be used. For example, arr[0] = 'new_value' and arr[-1] = 'new_value'.

View in Datadog  Leave us feedback  Documentation

start_time = Core::Utils::Time.now.utc # Save the start time as the span creation is delayed
begin
response = @handler.call(context)
rescue Exception => e
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Suggested change
rescue Exception => e
rescue StandardError => e
Do not rescue the Exception class (...read more)

The rule "Do not rescue the Exception class" is a crucial practice in Ruby programming for handling exceptions. The Exception class is the root of Ruby's exception hierarchy, so when you rescue Exception, you're potentially catching and handling severe system errors that Ruby itself is trying to bubble up. These could be fundamental issues like memory overflows and syntax errors, which could cause the program to behave unexpectedly or even crash.

Rescuing the Exception class can lead to major problems in debugging since it can hide the true nature of the error and its source. It makes it harder to pinpoint where and why the error occurred. This can lead to significant delays in identifying and resolving coding issues.

Instead of rescuing the Exception class, it is better to rescue more specific error classes or use StandardError which is the superclass for most error types. For instance, if you're expecting possible nil values, use rescue NoMethodError. This allows Ruby to handle severe system errors appropriately and ensures that you're only rescuing the errors you expect. This practice makes your code safer, more predictable, and easier to maintain and debug.

View in Datadog  Leave us feedback  Documentation

Comment on lines +401 to +406
message_attributes: {
'_datadog' => {
string_value: 'String',
data_type: 'String'
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Quality Violation

Consider wrapping this hash literal in braces (...read more)

In Ruby, hash literals can sometimes be ambiguous, especially when they are the last element in an array without being explicitly wrapped in braces. This rule enforces the use of braces around hash literals when they are the last element in an array.

The importance of this rule lies in maintaining the clarity and readability of your code. It can be confusing for others (or even for yourself at a later date) to understand the intention of your code when a hash literal is not clearly defined within an array. Furthermore, not wrapping a hash literal in braces may lead to unexpected behavior, as Ruby might not interpret it as you intended.

To adhere to this rule and ensure good coding practices, always wrap hash literals in braces {} when they are the last element in an array. For example, instead of writing [1, 42, foo: 99, bar: 96], you should write [1, 42, { foo: 99, bar: 96 }]. This makes it clear that the last element is a hash, improving the readability and predictability of your code.

View in Datadog  Leave us feedback  Documentation

@pr-commenter
Copy link

pr-commenter bot commented Sep 18, 2024

Benchmarks

Benchmark execution time: 2024-09-19 22:16:10

Comparing candidate commit 8f0bc5d in PR branch sns-sqs with baseline commit e32e038 in branch master.

Found 0 performance improvements and 1 performance regressions! Performance is the same for 22 metrics, 2 unstable metrics.

scenario:tracing - Propagation - Trace Context

  • 🟥 throughput [-3770.823op/s; -3644.676op/s] or [-9.885%; -9.554%]

@ivoanjo
Copy link
Member

ivoanjo commented Sep 19, 2024

image

This one seems to be a bit more "flaky-like" than the others -- opened #3928 to hopefully improve it.

@marcotc marcotc force-pushed the sns-sqs branch 3 times, most recently from ce3fd48 to 242c432 Compare September 19, 2024 21:28
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 99.09910% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.79%. Comparing base (e32e038) to head (8f0bc5d).
Report is 71 commits behind head on master.

Files with missing lines Patch % Lines
lib/datadog/tracing/contrib/aws/service/base.rb 87.50% 2 Missing ⚠️
lib/datadog/tracing/contrib/aws/service/sqs.rb 87.50% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #3842    +/-   ##
========================================
  Coverage   97.79%   97.79%            
========================================
  Files        1285     1288     +3     
  Lines       77028    77352   +324     
  Branches     3805     3824    +19     
========================================
+ Hits        75329    75647   +318     
- Misses       1699     1705     +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marcotc marcotc mentioned this pull request Sep 20, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants