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

improv(tracer): set AWS_XRAY_CONTEXT_MISSING to IGNORE_ERROR when no value is set #3058

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

dreamorosi
Copy link
Contributor

Summary

Changes

Please provide a summary of what's being changed

This PR updates the Tracer utility to set the AWS_XRAY_CONTEXT_MISSING environment variable to IGNORE_ERROR by default.

This change has been made in response to customers adopting features that rely on top-level await and finding the error logs produced by X-Ray SDK not useful.

The X-Ray SDK for Node.js was not designed with this feature in mind, and by default it logs an error when a code path attempts to send trace data outside of the context of a request.

This is because without the request context, there's no X-Ray Trace ID, and so the SDK is unable to establish the lineage of a segment/span.

In many cases this is a desired behavior (although I'd have preferred it was a warning rather than an error log), however now that making async operations during the init phase is possible, it's not uncommon to make outbound http requests or use the AWS SDK to retrieve data or perform setup operations.

In those cases, the auto instrumentation features of Tracer (& the underlying X-Ray SDK) attempt to create a segment for these operations but don't find one because these tasks are run during the init phase, and thus outside of the context of a request.

The change has been made with backwards compatibility in mind, to an extent. Before setting the value we check if the env variable is already set and if so, we leave it as-is.

I believe this is a good middle ground between furthering our features and maintaining flexibility.

Please add the issue number below, if no issue is present the PR might get blocked and not be reviewed

Issue number: closes #2406


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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@dreamorosi dreamorosi requested review from a team as code owners September 12, 2024 21:42
@boring-cyborg boring-cyborg bot added the tracer This item relates to the Tracer Utility label Sep 12, 2024
@pull-request-size pull-request-size bot added the size/S PR between 10-29 LOC label Sep 12, 2024
@dreamorosi dreamorosi self-assigned this Sep 12, 2024
@github-actions github-actions bot added the enhancement PRs that introduce minor changes, usually to existing features label Sep 12, 2024
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

For more coverage and to make sure that if there are any changes in the future, they will be detected, you should add a test (EnvVarTest) or is this too much? WDYT?

Requesting changes to hear from you.

@boring-cyborg boring-cyborg bot added the tests PRs that add or change tests label Sep 12, 2024
Copy link

@dreamorosi
Copy link
Contributor Author

Yea good point - I added it elsewhere because that one you linked is for reading from the env var, but I think it's the same.

@dreamorosi dreamorosi merged commit c0d2158 into main Sep 12, 2024
13 checks passed
@dreamorosi dreamorosi deleted the improv/tracer_xray_ctx_missing branch September 12, 2024 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that introduce minor changes, usually to existing features size/S PR between 10-29 LOC tests PRs that add or change tests tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: avoid tracing API calls made during init (top-level await)
2 participants