-
Notifications
You must be signed in to change notification settings - Fork 620
feat(aws-sdk): add exception hook #2398
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
feat(aws-sdk): add exception hook #2398
Conversation
dd9b3b3 to
117d248
Compare
117d248 to
8de2ca6
Compare
8de2ca6 to
8fe1574
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2398 +/- ##
=======================================
Coverage 89.84% 89.84%
=======================================
Files 188 188
Lines 9272 9282 +10
Branches 1901 1903 +2
=======================================
+ Hits 8330 8339 +9
- Misses 942 943 +1
🚀 New features to boost your workflow:
|
8fe1574 to
29b9dd2
Compare
|
@jj22ee can you please take a look at this? I know it was opened before you were made a component owner but it's been sitting for quite a while. |
|
I think this feature looks reasonable. The package already has similar hooks defined and it is well documented/tested. If it gets rebased I think we can move forward with it. @jj22ee please comment if you disagree otherwise this will get my 👍 |
|
Hey, sorry for not taking a look at this, thanks @dyladan for the ping. @project0 I suppose this solves #1765 indirectly for you because this allows you to update Span status or unrecord the Span exception via the new |
jj22ee
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.
Just a bit of cleanup. After you resolve the conflicts + merge/rebase with main I'll approve.
plugins/node/opentelemetry-instrumentation-aws-sdk/test/aws-sdk-v3.test.ts
Outdated
Show resolved
Hide resolved
|
Please resolve the conflicts and we can get this merged |
8586074 to
c9779e5
Compare
|
@dyladan sorry for the late response, i rebased everything and addressed the small linting issues. |
c9779e5 to
24bedb6
Compare
24bedb6 to
562f028
Compare
Which problem is this PR solving?
Short description of the changes