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.

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

Jump to bottom

SQS Integration (Aws Sdk) #546

Closed
wants to merge 9 commits into from
Closed

SQS Integration (Aws Sdk) #546

wants to merge 9 commits into from

Conversation

colin-higgins
Copy link
Member

@colin-higgins colin-higgins commented Nov 7, 2019 •
edited
Loading

Changes proposed in this pull request:

  • Introduce SQS client instrumentation from the AWS SDK
  • Tests verifying sample app sends spans
  • Test framework refactors

@DataDog/apm-dotnet

@colin-higgins colin-higgins added area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) area:tests unit tests, integration tests labels Nov 7, 2019
@colin-higgins colin-higgins requested a review from a team as a code owner November 7, 2019 14:10
@colin-higgins colin-higgins self-assigned this Nov 7, 2019
@colin-higgins colin-higgins changed the title Colin/integrations/aws sqs SQS Integration (Aws Sdk) Nov 7, 2019
@colin-higgins colin-higgins mentioned this pull request Nov 7, 2019
Merged
@colin-higgins colin-higgins added this to the 1.10.0 milestone Nov 7, 2019
@bobuva
Copy link
Contributor

bobuva commented Nov 7, 2019

@colin-higgins For the purpose of code review, it would be good to know the steps you went through to retrieve and set up the AWS SDK. One thing, for example, I'd like to do is verify that the integration points are complete, and being able to look at the AWS SDK assemblies would allow me to do that.

bobuva
bobuva suggested changes Nov 7, 2019
View reviewed changes
src/Datadog.Trace.ClrProfiler.Managed/Integrations/AWS/AmazonSQSIntegration.cs Outdated
var serviceName = $"{tracer.DefaultServiceName}-{ServiceName}";
scope = Tracer.Instance.StartActive(OperationName, serviceName: serviceName);
var span = scope.Span;
span.Type = SpanTypes.Http; // TODO, is this right?
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Amazon SQS is a queuing system, it seems that we should have a new SpanType.Queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would agree if this code were acting as the queue, but it is only the client for the queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

This did make me notice that our operation name seems to differ from java's implementation.
https://github.com/DataDog/dd-trace-java/blob/00fe40f1fba5c0df65a8af80430feb2a3cba6eea/dd-java-agent/instrumentation/aws-java-sdk-2.2/src/test/groovy/AwsClientTest.groovy#L78

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact they seem to have a mapping of operation names: https://github.com/DataDog/dd-trace-java/blob/42b88c7b8ec99b8b9ac2a4d4dda90ebbbe9e5921/dd-java-agent/instrumentation/aws-java-sdk-1.11.0/src/main/java/datadog/trace/instrumentation/aws/v0/AwsSdkClientDecorator.java#L37

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a consistency issue then. I would check for RFCs that apply to naming operations if such an RFC exists. And maybe asking in the apm-integrations Slack channel.

Copy link
Member Author

@colin-higgins colin-higgins Nov 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I worked with @tylerbenson to get a better idea of what Java is doing, so we can match up with them. I'll put up a draft RFC tomorrow sometime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, thanks @colin-higgins !

lucaspimentel
lucaspimentel reviewed Nov 18, 2019
View reviewed changes
test/Datadog.Trace.TestHelpers/MockTracerAgent.cs
@@ -146,42 +146,6 @@ protected virtual void OnRequestReceived(HttpListenerContext context)
RequestReceived?.Invoke(this, new EventArgs<HttpListenerContext>(context));
}

private static List<Span> ToSpans(dynamic data)
Copy link
Member

Choose a reason for hiding this comment

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

Should we merge the PR that this come from first so this (unrelated) diff is not here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with that.

Copy link
Member

Choose a reason for hiding this comment

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

#501

@lucaspimentel lucaspimentel modified the milestones: 1.10.0, 1.11.0 Nov 26, 2019
@lucaspimentel lucaspimentel removed this from the 1.11.0 milestone Dec 10, 2019
@lucaspimentel
Copy link
Member

Closing this PR for now. We will refactor this integration to use the new "call target" instrumentation in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zacharycmontoya zacharycmontoya zacharycmontoya left review comments

@lucaspimentel lucaspimentel lucaspimentel left review comments

@bobuva bobuva bobuva left review comments

Assignees

@colin-higgins colin-higgins

Labels
area:automatic-instrumentation Automatic instrumentation managed C# code (Datadog.Trace.ClrProfiler.Managed) area:tests unit tests, integration tests status:on-hold type:new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@colin-higgins @bobuva @lucaspimentel @zacharycmontoya