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

Instrument RPC calls to amazon SQS #933

Merged
merged 7 commits into from Apr 15, 2021

Conversation

stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Apr 14, 2021

Add instrumentation for SQS methods for sending/receiving/deleting messages.

Note:
Ignoring message queues is part of the messaging spec, but will be included later. This is being tracked in #934.

closes #883

@stuartnelson3 stuartnelson3 requested a review from a team April 14, 2021 09:08
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Apr 14, 2021
@apmmachine
Copy link
Collaborator

apmmachine commented Apr 14, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #933 updated

  • Start Time: 2021-04-15T09:36:01.549+0000

  • Duration: 28 min 30 sec

  • Commit: 0de1b64

Test stats 🧪

Test Results
Failed 0
Passed 10173
Skipped 266
Total 10439

Trends 🧪

Image of Build Times

Image of Tests

@stuartnelson3 stuartnelson3 marked this pull request as ready for review April 14, 2021 09:26
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Looks good, with the caveat that I don't really know SQS. We should be recording trace context (traceparent & tracestate) in message attributes.

module/apmawssdkgo/sqs.go Show resolved Hide resolved
Add the trace context header to the message
attribute metadata when creating messages.
@stuartnelson3
Copy link
Contributor Author

stuartnelson3 commented Apr 14, 2021

I added the trace context to the message attributes, but when I was inspecting the body for SendMessage it seems like it's tripled, which is a bit worrisome:

Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageAttribute.2.Name=traceContext
MessageAttribute.2.Value.DataType=String
MessageAttribute.2.Value.StringValue=00-a766982841306178091fb3f9c0c1e5b5-a49abb74fdf1b46a-01
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageAttribute.2.Name=traceContext
MessageAttribute.2.Value.DataType=String
MessageAttribute.2.Value.StringValue=00-a766982841306178091fb3f9c0c1e5b5-a49abb74fdf1b46a-01
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageAttribute.2.Name=traceContext
MessageAttribute.2.Value.DataType=String
MessageAttribute.2.Value.StringValue=00-a766982841306178091fb3f9c0c1e5b5-a49abb74fdf1b46a-01
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05

I removed all the instrumentation we do (don't add build, send, complete to the handlers list), and it's still showing duplicates, so maybe it's a bug..?

Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05Action=SendMessage
MessageAttribute.1.Name=attr
MessageAttribute.1.Value.DataType=String
MessageAttribute.1.Value.StringValue=string+attr
MessageBody=msg+body
QueueUrl=https%3A%2F%2Fsqs.testing.invalid%2F123456789012%2FOtherQueue
Version=2012-11-05

This is not happening in SendMessageBatch. I'm not sure what might be causing this from our end.

module/apmawssdkgo/session.go Show resolved Hide resolved
module/apmawssdkgo/sqs.go Outdated Show resolved Hide resolved
module/apmawssdkgo/sqs.go Outdated Show resolved Hide resolved
module/apmawssdkgo/sqs_test.go Outdated Show resolved Hide resolved
if tc.hasTraceContext {
wrapped.Handlers.Build.PushBackNamed(request.NamedHandler{
Name: "spy_message_attrs_added",
Fn: testTracingAttributes(t),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Took some inspiration from the ruby client test and added another handler to inspect that the params were correctly modified.

Copy link
Member

Choose a reason for hiding this comment

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

nice!

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

looks good!

@stuartnelson3 stuartnelson3 merged commit 95c2457 into elastic:master Apr 15, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Apr 15, 2021
@stuartnelson3 stuartnelson3 deleted the aws-sqs branch April 15, 2021 12:51
stuartnelson3 added a commit that referenced this pull request Jul 30, 2021
Add support for tracing AWS SQS RPCs when using a wrapped `*session.Session` struct
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

[META 408] Instrumentation for AWS SQS
3 participants