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

feat(all): moved EnvService to commons + exposed getXrayTraceId in tracer #1123

Merged
merged 5 commits into from Oct 27, 2022

Conversation

dreamorosi
Copy link
Contributor

@dreamorosi dreamorosi commented Oct 17, 2022

Description of your changes

In #1070 customers have requested the addition of a new method in Tracer that allows to access the AWS X-Ray Root Trace ID. This is useful in those situations in which customers want to use this id as correlation id in the responses returned by a function or manually pass it to downstream processes.

The logic to retrieve and parse this ID was already present in Logger (here). For that utility we use the method to append the xray_trace_id to log entries in some cases.

During the discussion of the feature it became clear that this could be a good moment to also extract this logic to the commons package continuing the work started in #484, and potentially unlocking/facilitating #165.

Before this change

image

After this change

image

This issue also introduces a new public method in Tracer called getRootXrayTraceId that allows customers to access the AWS X-Ray Root Trace ID within a Lambda handler.

Fixes #1070

How to verify this change

Check the unit tests results, as well as package size report, and e2e tests.

Related issues, RFCs

Issue number: #1070

PR status

Is this ready for review?: YES
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
  • 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 in downstream module
  • The PR title follows the conventional commit semantics

Breaking change checklist

N/A


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

@dreamorosi dreamorosi self-assigned this Oct 17, 2022
@dreamorosi dreamorosi linked an issue Oct 17, 2022 that may be closed by this pull request
@github-actions
Copy link
Contributor

📊 Package size report   3%↑

File Before After
aws-lambda-powertools-commons-1.3.0.tgz 6.2 kB 51%↑9.4 kB
aws-lambda-powertools-logger-1.3.0.tgz 24.6 kB -3.7%↓23.7 kB
aws-lambda-powertools-metrics-1.3.0.tgz 17.8 kB -1.17%↓17.6 kB
aws-lambda-powertools-tracer-1.3.0.tgz 22.5 kB 2%↑22.9 kB
commons-bundle.zip 6.8 kB 46%↑9.9 kB
logger-bundle.zip 25.1 kB -3.51%↓24.2 kB
metrics-bundle.zip 18.3 kB -1.28%↓18.1 kB
tracer-bundle.zip 23.1 kB 1%↑23.4 kB
Total (Includes all files) 144.4 kB 3%↑149.2 kB
Tarball size 143.1 kB 3%↑148.1 kB

🤖 This report was automatically generated by pkg-size-action
(options hash: 45418e540c1efd5043014571ffc11654)

@dreamorosi
Copy link
Contributor Author

@dreamorosi dreamorosi marked this pull request as ready for review October 18, 2022 11:57
@dreamorosi dreamorosi added this to Pull Requests - Work in progress in Pull Requests via automation Oct 18, 2022
Copy link
Contributor

@misterjoshua misterjoshua left a comment

Choose a reason for hiding this comment

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

This PR looks pretty good to me. For now, I have just a few minor comments.

docs/core/tracer.md Outdated Show resolved Hide resolved
packages/logger/src/config/EnvironmentVariablesService.ts Outdated Show resolved Hide resolved
packages/tracer/src/config/EnvironmentVariablesService.ts Outdated Show resolved Hide resolved
Co-authored-by: Josh Kellendonk <misterjoshua@users.noreply.github.com>
@dreamorosi dreamorosi moved this from Pull Requests - Work in progress to Pull Requests - Review needed in Pull Requests Oct 19, 2022
misterjoshua
misterjoshua previously approved these changes Oct 21, 2022
Copy link
Contributor

@misterjoshua misterjoshua left a comment

Choose a reason for hiding this comment

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

This all looks good to me!

@dreamorosi
Copy link
Contributor Author

@misterjoshua Thanks a lot for taking the time to review my PR, appreciate your time!

saragerion
saragerion previously approved these changes Oct 27, 2022
Copy link
Contributor

@saragerion saragerion left a comment

Choose a reason for hiding this comment

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

LGTM!

flochaz
flochaz previously approved these changes Oct 27, 2022
Copy link
Contributor

@flochaz flochaz left a comment

Choose a reason for hiding this comment

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

LGTM, minor update on UT would b great


describe('Method: getXrayTraceId', () => {

test('It returns the value of the environment variable _X_AMZN_TRACE_ID', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add better test for this logic:

  1. if empty ?
  2. if real (see the split works)

Pull Requests automation moved this from Pull Requests - Review needed to Pull Requests - Approved and ready to be merged Oct 27, 2022
Pull Requests automation moved this from Pull Requests - Approved and ready to be merged to Pull Requests - Review needed Oct 27, 2022
@dreamorosi dreamorosi merged commit c8e3c15 into main Oct 27, 2022
Pull Requests automation moved this from Pull Requests - Review needed to Pull Requests - Merged or Closed Oct 27, 2022
@dreamorosi dreamorosi deleted the 1070-feature-tracer-access-to-the-root-trace-id branch October 27, 2022 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Pull Requests
Pull Requests - Merged or Closed
Development

Successfully merging this pull request may close these issues.

Feature request: access to the root trace id in Tracer
4 participants