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

refactor(tracer): log warning instead of throwing when segment is not found #1370

Merged
merged 8 commits into from
Mar 20, 2023

Conversation

dreamorosi
Copy link
Contributor

Description of your changes

As reported in #1356, the current behavior of the Tracer and corresponding middleware is fairly unforgiving when it comes to handling missing segments. Prior to this PR, when calling the getSegment method, if a segment is not found, an error would be thrown.

This can happen either when users try to retrieve the current segment outside of the context of a function invocation or when the trace data is malformed and the X-Ray SDK is not able to derive the current segment.

This behavior, if not handled properly, can cause a function invocation to fail, which might not be desirable given that for many customers tracing is considered best-effort.

This PR:

  • aligns the Tracer utility with the X-Ray SDK so that if a segment is not found, a warning is logged instead of throwing
  • improves the handling of missing segments in the captureLambdaHandler middleware in all stages to be more resilient
  • bumps the aws-xray-sdk-node dependency to the latest version
  • refactor and extracts some of the internal logic around current segment retrieval from the Tracer class to the ProviderService one

This PR closes #1356.

How to verify this change

Check the existing tests as well as the newly added ones in the checks under this repo. Check also the results of this successful integration test.

Related issues, RFCs

Issue number: #1356

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
  • I have made corresponding changes to the examples
  • 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
  • The PR title follows the conventional commit semantics

Breaking change checklist

Is it a breaking change?: NO

  • 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.

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

@dreamorosi dreamorosi self-assigned this Mar 13, 2023
@pull-request-size pull-request-size bot added the size/XL PRs between 500-999 LOC, often PRs that grown with feedback label Mar 13, 2023
@github-actions github-actions bot added the feature PRs that introduce new features or minor changes label Mar 13, 2023
@am29d am29d self-requested a review March 14, 2023 10:37
Copy link
Contributor

@am29d am29d left a comment

Choose a reason for hiding this comment

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

I like the cleanup it tests 👌, much easier to follow now.

The implications of additional undefined are bigger than I thought. It tears through many methods, similar to isTracingEnabled and reminds me of java or golang null checks. But I have no immediate answer to make it cleaner.

packages/tracer/src/Tracer.ts Show resolved Hide resolved
packages/tracer/src/Tracer.ts Outdated Show resolved Hide resolved
@dreamorosi
Copy link
Contributor Author

I like the cleanup it tests 👌, much easier to follow now.

Thanks!

The implications of additional undefined are bigger than I thought. It tears through many methods, similar to isTracingEnabled and reminds me of java or golang null checks. But I have no immediate answer to make it cleaner.

Indeed, my attempt at somewhat centralising this behavior was to extract it in the provider class. This is also something I've been meaning to do for a while as it'd help decouple the two and open the door for other providers in the future.

@dreamorosi dreamorosi requested a review from am29d March 17, 2023 17:02
@dreamorosi dreamorosi merged commit a82fc3a into main Mar 20, 2023
@dreamorosi dreamorosi deleted the 1356-tracer-middleware-error branch March 20, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature PRs that introduce new features or minor changes size/XL PRs between 500-999 LOC, often PRs that grown with feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Tracer should log warning instead of throwing when segment is not found
2 participants