-
Notifications
You must be signed in to change notification settings - Fork 369
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
Consistent APM Span tags for AWS SDK Requests #2730
Conversation
d3a79e0
to
4496b3a
Compare
…requests, and move tag keys to constants file
…ferent one for ruby version < 2.2.0 as the host differs
updated gem files as I believe that may have been why the tests were failing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned, we would like to address the test failures for aws-sdk 2.x, since it is aligned with our support policy
@TonyCTHsu @marcotc Addressed your previous round of comments, thank you! |
@@ -18,7 +18,17 @@ module Ext | |||
TAG_OPERATION = 'aws.operation'.freeze | |||
TAG_OPERATION_COMMAND = 'command'.freeze | |||
TAG_PATH = 'path'.freeze | |||
TAG_REGION = 'aws.region'.freeze | |||
TAG_AWS_REGION = 'aws.region'.freeze | |||
TAG_REGION = 'region'.freeze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to achieve pivoting from metrics to traces to work out of the box we need these new tags. For instance if we wish to pivot from aws integration metric aws.dynamodb.table_size
then our spans metadata needs to match with metrics metadata. This also allows us to unlock benefit of inferring related resources on the serverless function page.
Why not remove old tags such as aws.region, or aws.sqs.queue_name now? Many of these tags are already used by customer monitors, dashboards, metrics, notebooks... Therefore I have logged a jira ticket under framework integrations to eventually phase these old tags out.
If the aws integrations metrics could change their metric tag values then this would solve the pivot issue but changing a tag on a metric like that is a much bigger breaking change as much more customer resources rely on those and they are stored for much longer to my knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to individual files looks good, and also the testing being split on a per-aws service basis.
I left a suggestion about how to structure this new code refactor so it more seamlessly integrate with our library code base.
The main theme here is that it looks like we are simulating polymorphism with a case/when
statement, so I suggested a different approach.
|
||
require_relative '../ext' | ||
|
||
def add_dynamodb_tags(span, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of these service-specific files would look like:
module Datadog
module Tracing
module Contrib
module Aws
module Service
class DynamoDB < Base
def add_tags(span, params)
table_name = params[:table_name]
span.set_tag(Aws::Ext::TAG_TABLE_NAME, table_name)
end
end
end
end
end
end
end
You'd also create a base implemented, just in case case we want a fallback behaviour (in the base.rb
file):
module Datadog
module Tracing
module Contrib
module Aws
module Service
class Base
def add_tags(span, params)
end
end
end
end
end
end
end
aws_service = span.resource.split('.')[0] | ||
span.set_tag(Ext::TAG_AWS_SERVICE, aws_service) | ||
params = context.safely(:params) | ||
add_service_specific_tags(span, aws_service, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it would look similar to:
if (handler = Aws::SERVICE_HANDLER[aws_service])
handler.add_tags(span, params)
end
case aws_service | ||
when 'sqs' | ||
add_sqs_tags(span, params) | ||
when 'sns' | ||
add_sns_tags(span, params) | ||
when 'dynamodb' | ||
add_dynamodb_tags(span, params) | ||
when 'kinesis' | ||
add_kinesis_tags(span, params) | ||
when 'eventbridge' | ||
add_eventbridge_tags(span, params) | ||
when 'states' | ||
add_states_tags(span, params) | ||
when 's3' | ||
add_s3_tags(span, params) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block would be removed, and effectively replaced by a new table SERVICE_HANDLERS
, added to the existing services.rb
file:
module Datadog
module Tracing
module Contrib
module Aws
SERVICES = %w[
# ...
].freeze
SERVICE_HANDLERS = {
'sqs' => Service::SQS.new,
'sns' => Service::SNS.new,
# ...
}
end
end
end
end
@@ -0,0 +1,8 @@ | |||
# frozen_string_literal: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For namespacing, in Ruby, it's standard practice to use singular names for folders, so I recommend renaming this directory from aws/services
to aws/service
.
|
||
require_relative '../ext' | ||
|
||
def add_kinesis_tags(span, params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a side-point, immaterial after the proposed changes: a method declaration like this, at the top of the file, not inside any class
or module
creates a global function in the current process.
This means, for example, add_kinesis_tags
is globally available for the client to invoke, from pretty much anywhere in their code base. This not desirable for a library like ours to do because it pollutes the application naming space.
In the worst case scenario, the user has a add_kinesis_tags
method already and we just overwrote it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
funny enough I ran into this while testing and thats why I made the functions unique but I didn't know it could interfere with a users naming space. All good lessons learned. Thank you
…services to service directory
Adds
Motivation
sls-3240
attempted to add
aws-account
but no luck with all services as the aws-sdk params change with each type of request. I'd be interested in hearing thoughts on this.Serverless inferred related resources, pivot from traces to aws integration metrics ootb, see this doc
Consistent APM span tags for AWS requests
(search this document in Google Cloud Search)Testing
Unit tests with stubbed aws-sdk requests