-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Tracing capabilities for @sentry/serverless. #2945
Tracing capabilities for @sentry/serverless. #2945
Conversation
@marshall-lee ping me once it's done or just ready for a review (not sure how you personally use drafts) |
bc19693
to
a3620c9
Compare
@kamilogorek It's ready for review! Few notes from me:
|
Hi. To be honest idk what i must do. Can you explain for me. What this and
for what. And how cheng
language for Russian. Thanks
пн, 5 окт. 2020 г., 20:10 Vladimir Kochnev <notifications@github.com>:
… @kamilogorek <https://github.com/kamilogorek> It's ready for review!
Few notes from me:
- Not sure where to put awsresources module. Now it resides in
packages/serverless but in general it could be used anywhere, it's not
strictly required to use it only with aws lambda.
- I wanted to re-define defaultIntegrations() for @sentry/serverless
and put new AWSResources() there but @sentry/serverless is not just
for aws lambda. What do you think about making
@sentry/serverless/awslambda export all the @sentry/node stuff but
with overridden defaultIntegrations()?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2945 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQ44LM7TP2VRY5HM2QDVHWTSJH4W3ANCNFSM4R7L7OAQ>
.
|
7a9bb4d
to
19b6573
Compare
19b6573
to
0a618a4
Compare
041b8b3
to
fd57745
Compare
@marshall-lee Can I give it another pass? |
69d33a4
to
f928566
Compare
yes |
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 👍
Awesome stuff, especially the tests.
The only thing I would ask (since I don't have a test-setup now), could you provide screenshots of Transactions in Sentry to see this in action and how it looks like?
f928566
to
a514eb8
Compare
@HazAT Thanks! I will provide a series of screencasts later, I'm working on it. |
Some demos:
|
598aea5
to
c8a90f0
Compare
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.
Awesome work, thank you! My questions are just nitpicking, so other than that this PR should be good to go.
packages/serverless/README.md
Outdated
If you also want to trace performance of all the incoming requests and also outgoing AWS service requests, just set the `tracesSampleRate` option. | ||
|
||
```javascript | ||
import * as Sentry from '@sentry/serverless/awslambda'; |
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.
I believe it should either say:
import * as Sentry from '@sentry/serverless/awslambda';
Sentry.init(...);
or
import * as Sentry from '@sentry/serverless';
Sentry.AWSLambda.init(...);
What about tree-shaking? If one is using AWSLambda, will it strip the GCPFunction code down?
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.
import * as Sentry from '@sentry/serverless/awslambda'
was a typo. It was intended to be:
import * as Sentry from '@sentry/serverless';
Sentry.AWSLambda.init(...);
As for tree-shaking, I need to check it.
c8a90f0
to
86d806a
Compare
This integration traces AWS service calls as spans.
86d806a
to
e899040
Compare
@marshall-lee it tree-shakes perfectly, you don't need to verify it anymore. Thanks! |
Add tracing support for AWS Lambda & Google Cloud Functions.
Also I believe it fixes #2956.