-
Notifications
You must be signed in to change notification settings - Fork 139
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
fix: updated CDK examples to remove old references & improve comments #439
Conversation
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 only major one is that we have try/catch without throwing an error. I am willing approve if that is addressed.
The rest are either minor/nit.
try { | ||
throw new Error('test'); | ||
// Add the response as metadata | ||
res = { foo: 'bar' }; | ||
} catch (err) { |
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.
Same as above. Why try/catch without throw?
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.
See this 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.
This one has the throw at L57.
examples/cdk/lib/example-function.Tracer.CaptureErrorDisabled.ts
Outdated
Show resolved
Hide resolved
const sts = tracer.captureAWSClient(new STS()); | ||
|
||
// Here we are showing an example with manual instrumentation but you can do the same also with the captureLambdaHandler Middy Middleware and Class decorator |
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.
(minor) I am not aware of this. O.o
AFAI see, this is not mentioned in the documentation. Can we add it to the list in this section . Another PR is fine.
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 actual example, in this file, is from L4 to L11 since this example wants to demonstrate how to instrument an AWS SDK v2 client.
What I wanted to say with this comment is that even though this example uses manual instrumentation, you don't need to use this method specifically over others (Middy middleware, decorator). Essentially, as long as you instrument the AWS SDK client with tracer.capture*
it'll be traced & appear as custom subsegment on X-Ray no matter how you write your function.
That's why in the docs the AWS SDK patching example only has these lines.
I included a function in the example just because otherwise the lines above alone wouldn't run as a Lambda function and the lines from L15 onwards are similar to manual method in the section you linked.
…bled.ts Co-authored-by: ijemmy <ijemmy@users.noreply.github.com>
Co-authored-by: ijemmy <ijemmy@users.noreply.github.com>
…rtools-typescript into fix/cdk_examples
As CDK V2 is GA, would it make sense to just start using it from the get go @dreamorosi ? |
Would love to, but at the moment we are relying on some classes that are not exported in v2, see this issue I've opened on the CDK repo. |
Description of your changes
This PR sets up #425 and aims at:
How to verify this change
After #416 has been merged,checkout the branch, go toexamples/cdk
& runcdk deploy
.Related issues, RFCs
#435
#425
PR status
Is this ready for review?: YES
Is it a breaking change?: NO
Checklist
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.