-
Notifications
You must be signed in to change notification settings - Fork 181
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
Conversation
|
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 |
ijemmy
left a comment
There was a problem hiding this 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}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Should it have "##"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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).
saragerion
left a comment
There was a problem hiding this 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! 🎉
saragerion
left a comment
There was a problem hiding this 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:
Co-authored-by: Sara Gerion <[email protected]>
Co-authored-by: Sara Gerion <[email protected]>
Co-authored-by: Sara Gerion <[email protected]>
Co-authored-by: Sara Gerion <[email protected]>

Description of your changes
This PR includes the following changes, all related to the
Tracermodule:@aws-lambda-powertools-typescript/commonspackage in tests (examples will be updated in separate PR related to Maintenance: delete examples in packages folder #332).README.mdfile of the package to include only intro + link to main docs as discussed in standup.tracer.addResponseAsMetadataas public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.addErrorAsMetadataas public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.annotateColdStartas public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.annotateColdStartso that annotation is always added to main segment even when it's not a cold start, this is a change mentioned here Feature request: tracer feature improvements #275.tracer.addServiceNameAnnotationto allow Customers to add the annotation to the main segment, changed middleware & decorator to use it, updated tests & docs; this is a change mentioned here Feature request: tracer feature improvements #275.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
Breaking change checklist
This is a breaking change in the sense that generated traces will now all have
ColdStart(sometimestrue, sometimesfalse) andServiceNameannotations.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.