Skip to content

chore(tracer): quality of life improvements #337

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

Merged
merged 15 commits into from
Dec 23, 2021
Merged

Conversation

dreamorosi
Copy link
Contributor

Description of your changes

This PR includes the following changes, all related to the Tracer module:

How to verify this change

Verify that all tests are passing & coverage hasn't decreased.

Related issues, RFCs

#332
#334
#275
#851 (Python)
#861 (Python)

PR status

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

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)

This is a breaking change in the sense that generated traces will now all have ColdStart (sometimes true, sometimes false) and ServiceName annotations.


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 added enhancement tracer This item relates to the Tracer Utility labels Dec 17, 2021
@dreamorosi dreamorosi added this to the beta-release milestone Dec 17, 2021
@dreamorosi dreamorosi self-assigned this Dec 17, 2021
@dreamorosi
Copy link
Contributor Author

I revisited the implementation of the Tracer middleware because while testing it on an actual Lambda function I was running in an issue similar to the one described here. The integration and API haven't changed but the implementation now correctly handles the segments, the key was in calling setSegment with the facade segment (the main one) at the end so that each execution in the same execution environment is represented correctly in X-Ray.

ijemmy
ijemmy previously approved these changes Dec 21, 2021
Copy link
Contributor

@ijemmy ijemmy left a comment

Choose a reason for hiding this comment

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

LGTM overall. I have only a few nits for you to double check.


const open = (): void => {
lambdaSegment = target.getSegment();
const handlerSegment = lambdaSegment.addNewSubsegment(`## ${process.env._HANDLER}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Should it have "##"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't have a strong feeling about this, followed Python's implementation that puts ## in subsegments (see img below - taken from docs here or Python implementation here)
tracer_utility_showcase.

Copy link
Contributor

Choose a reason for hiding this comment

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

The python tracer library is opinionated on the syntax of the subsegments' labels (as it is with the format of the structure logs, for example).
I say that for consistency and feature parity we should leave it as it is, but allow customers to control the annotation labels if they want to in the future (but this we can do after the beta).

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.

Added a couple of comments (minor). Thanks @dreamorosi! 🎉

@dreamorosi dreamorosi linked an issue Dec 22, 2021 that may be closed by this pull request
3 tasks
ijemmy
ijemmy previously approved these changes Dec 22, 2021
saragerion
saragerion previously approved these changes Dec 22, 2021
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.

Looks great, I just left a minor nit (I would find/replace in the codebase for captureLambdaHanlder), but for me it's already good to merge :chefskiss:

@dreamorosi dreamorosi dismissed stale reviews from saragerion and ijemmy via 23e5a36 December 23, 2021 09:01
@dreamorosi dreamorosi merged commit ad4b941 into main Dec 23, 2021
@dreamorosi dreamorosi deleted the chore/tracer_qol branch December 23, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracer This item relates to the Tracer Utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: feature parity for manual instrumentation tracer
3 participants