-
Notifications
You must be signed in to change notification settings - Fork 154
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 |
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).
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! 🎉
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
Tracer
module:@aws-lambda-powertools-typescript/commons
package in tests (examples will be updated in separate PR related to Maintenance: delete examples in packages folder #332).README.md
file of the package to include only intro + link to main docs as discussed in standup.tracer.addResponseAsMetadata
as public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.addErrorAsMetadata
as public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.annotateColdStart
as public API, changed middleware to use it, updated tests & docs (Feature request: feature parity for manual instrumentation tracer #334).tracer.annotateColdStart
so 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.addServiceNameAnnotation
to 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
) andServiceName
annotations.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.