Skip to content

feat(tracer): Add capability to continue traces comming from SQS triggers #364

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

Closed
wants to merge 19 commits into from

Conversation

flochaz
Copy link
Contributor

@flochaz flochaz commented Dec 29, 2021

🚧 WIP 🚧 :

  • Missing proper unit and e2e tests assertions
  • Need to evaluate the API design :
    • is continueSQSRecordTrace and the way to invoke it good enough ?
    • Should we store toCloseSegments on our side and provide a function to close them ?
  • can we really can't do better in term of implementation ???

Description of your changes

This PR is a first draft trying to give a solution to the Xray node sdk issue aws/aws-xray-sdk-node#208 where traces should be continued when lambdaare triggered from SQS: Producer -> SQS -> Consumer Lambda -> ... (Today it creates 2 traces: Producer -> SQS and Lambda -> ...).

Here are the findings while implementing this:

  • aws-xray-sdk-node has a specific behavior for lambda based on process.env.LAMBDA_TASK_ROOT (https://github.com/aws/aws-xray-sdk-node/blob/master/packages/core/lib/aws-xray.js#L381)
  • Even if active tracing is disabled on the consumer lambda a trace is created for the Consumer Lambda
  • If XRay permission is not granted to consumer then no traces are created.
  • We can force env var _X_AMZN_TRACE_ID but
    • need to be set before import of the xray-sdk-core
    • it does not prevent the extra trace to be created with a different traceID
    • It does not help with the multi message use case

How to verify this change

cd packages/tracing
export DISABLE_TEARDOWN=true
npx jest --group=e2e/tracer/sqs

Check Xray for traces
Screenshot 2021-12-29 at 10 18 44

Or Cloudwatch Service Map
Screenshot 2021-12-29 at 10 20 07
Screenshot 2021-12-29 at 10 20 24

Related issues, RFCs

#286

PR status

Is this ready for review?: NO
Is it a breaking change?: NO

Checklist

  • My changes meet the tenets criteria
  • I have performed a self-review of my own code
  • I have commented my code where necessary, particularly in areas that should be flagged with a TODO, or hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • The code coverage hasn't decreased
  • I have added tests that prove my change is effective and works
  • New and existing unit tests pass locally and in Github Actions
  • Any dependent changes have been merged and published in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

  • I have documented the migration process
  • I have added, implemented necessary warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@flochaz flochaz marked this pull request as draft December 29, 2021 09:15
@dreamorosi dreamorosi added this to the production-ready-release milestone Jan 3, 2022
@heitorlessa heitorlessa changed the title feat(tracer): Add capability to continue traces comming from SQS triggers [WIP] feat(tracer): Add capability to continue traces comming from SQS triggers Jan 5, 2022
Comment on lines +146 to +163
const record: SQSRecord = {
messageId: 'fd95260b-1600-4028-b252-590cfcc9fe6d',
receiptHandle: 'test',
body: 'Information about current NY Times fiction bestseller for week of 12/11/2016.',
attributes: {
ApproximateReceiveCount: '1',
AWSTraceHeader: 'Root=1-61cc1005-53ff3b575736e3c74eae6bfb;Parent=1f57c53badf96998;Sampled=1',
SentTimestamp: '1640763398126',
SenderId: 'AROAT26JIZQWSOCCOUNE5:sqsProducer',
ApproximateFirstReceiveTimestamp: '1640763398127',
},
messageAttributes: {},
md5OfBody: 'bbdc5fdb8be7251f5c910905db994bab',
eventSource: 'aws:sqs',
eventSourceARN:
'arn:aws:sqs:eu-west-1:123456789012:queue',
awsRegion: 'eu-west-1',
};

Choose a reason for hiding this comment

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

ref

@flochaz
Copy link
Contributor Author

flochaz commented Jan 18, 2022

Waiting for X-Ray Service team strategy. Closing for now ...

@flochaz flochaz closed this Jan 18, 2022
@dreamorosi dreamorosi deleted the feat/tracer/sqs branch January 13, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants