[golang] Add go endpoints for SNS and SQS #3204
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nhulston
changed the title
nhulston
changed the title
nhulston
changed the title
nhulston
force-pushed
the
nicholas.hulston/serverless-update-go-endpoints
branch
from
4efb6d2 to
a52a549
Compare
wconti27
approved these changes
Oct 11, 2024
Contributor
wconti27
left a comment
wconti27
left a comment
There was a problem hiding this comment.
Looks great to me, thanks for this work!
cbeauchesne
approved these changes
Oct 11, 2024
nhulston
added a commit
to DataDog/dd-trace-dotnet
that referenced
this pull request
Oct 15, 2024
…ext (#6096) ## Summary of changes This creates a new instrumentation for EventBridge and intercepts `PutEvents` and `PutEventsAsync` to inject trace context. This allows the agent to combine spans from a distributed (serverless) architecture into a single trace. This PR only injects trace context. I'm working on [PR 1](DataDog/datadog-agent#29414) and [PR 2](DataDog/datadog-agent#29551) to update the Lambda extension to use this trace context to create EventBridge spans. I am also working on a similar PR in [dd-trace-java](DataDog/dd-trace-java#7613) and dd-trace-go. ## Reason for change SNS and SQS are already supported, and the tracer currently injects trace context into message attributes fields for them. However, EventBridge wasn't supported, and this PR aims to fix this problem. ## Implementation details I followed the [documentation](https://github.com/DataDog/dd-trace-dotnet/blob/master/docs/development/AutomaticInstrumentation.md) to create an instrumentation. Much of the logic was mirrored from the [existing implementation](https://github.com/DataDog/dd-trace-dotnet/tree/master/tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/AWS/SNS) of SNS, since EventBridge and SNS are extremely similar. Overall, AWS's EventBridge API is lacking some features, so we have to do some hacky solutions. - SNS and SQS call their custom input field messageAttributes, and EventBridge calls it detail - Unlike SNS and SQS, the detail field is given as a raw string. Therefore, we have to manually modify the detail string using StringBuilder. - The agent has no reliable way of getting the start time of the EventBridge span, so the tracer has to put the current time into `detail[_datadog]` under the header `x-datadog-start-time` - The EventBridge API has no way of getting the EventBridge bus name, so the tracer has to put the bus name (which is used to create the span resource name) into `detail[_datadog]` under the header `x-datadog-resource-name` ## Test coverage I added system tests for SNS/SQS: DataDog/system-tests#3204 I added [unit tests](d05eb4c) and [integration tests](5ccd8b7). Unit tests can be ran with: ``` cd tracer dotnet test ./test/Datadog.Trace.ClrProfiler.Managed.Tests ``` Integration tests can be ran with these commands: ``` cd tracer # Start docker localstock docker run --rm -it -p 4566:4566 -p 4571:4571 -e SERVICES=events localstack/localstack # Run integation tests ./build.sh BuildAndRunOSxIntegrationTests -buildConfiguration Debug -framework net6.0 -Filter AwsEventBridgeTests -SampleName Samples.AWS.EventBridge ``` I also did manual testing: <img width="505" alt="Screenshot 2024-09-30 at 11 00 47 AM" src="https://github.com/user-attachments/assets/bdf5d516-8b46-4138-ac25-c45d1822dc56"> ## Other details There are lots of diffs and files changed. I recommend reviewers to review the PR commit by commit. All the autogenerated files were added in a single commit, which should make the review process less overwhelming. <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. --> --------- Co-authored-by: Steven Bouwkamp <[email protected]>
6 tasks
cbeauchesne
requested changes
Oct 22, 2024
Collaborator
cbeauchesne
left a comment
cbeauchesne
left a comment
There was a problem hiding this comment.
There are failure related to the change
Collaborator
|
Hi @nhulston , Do you still plan to work on this PR? |
Contributor
Author
Hey, sorry for the late response. I'm going to close this PR, may revisit in the future |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
I have a pull request (DataDog/dd-trace-go#2917) to add trace injection into SQS, SNS, and EventBridge in the Go tracer. This PR aims to implement and enable the
producetests for SQS and SNS.Note: These changes in dd-trace-go are intended for the serverless team, so I haven't added any code to consume (extract context). This is because for serverless, context extraction is handled by the Lambda extension in datadog-agent.
Changes
This PR adds endpoints to
net-http:/sns/produce,/sns/consume,/sqs/produce,/sqs/consume.I enabled the
producetests for SNS and SQS for the Go tracer. However, I didn't enable anyconsumetests because trace extraction is not supported in Go yet.Since I was already making the produce endpoints for SNS and SQS, I thought I might as well add the consume endpoints to make someone's life easier in the future. However, the consume tests are expected to fail right now, so I haven't had the opportunity to test them (but I'm pretty sure they won't need to be changed in the future, since I copied all the logic for the endpoints from Python: https://github.com/DataDog/system-tests/blob/main/utils/build/docker/python/flask/app.py)
Note: Right now, the SQS and SNS tests are temporarily disabled
so the jobs aren't actually testing these changes. However, I enabled them locally and tested manually, and the
consumetests pass as expected and theproducetests fail as expected.To test, comment out the irrelevant tags and use the commands:
Workflow
codeownersfile quickly.🚀 Once your PR is reviewed, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
[<language>], double-check that only<language>is impacted by the changeNo system-tests internal is modified. Otherwise, I have the approval from R&P teamA docker base image is modified?build-XXX-imagelabel is presentA scenario is added (or removed)?