Skip to content
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

Stop throwing unnecessary errors with adding annotations, metadata, errors #467

Merged
merged 3 commits into from
Jun 12, 2022
Merged

Stop throwing unnecessary errors with adding annotations, metadata, errors #467

merged 3 commits into from
Jun 12, 2022

Conversation

wparad
Copy link
Contributor

@wparad wparad commented Oct 10, 2021

Description of changes:
Adding annotations, metadata, and errors with malformed data, will be logged as an error instead of hard crashing the caller. XRay Segments are not production critical code and therefore should never throw an exception when it can be avoided. This changes default values and keys to something xray can handle, but won't crash the caller.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@NathanielRN
Copy link
Contributor

Thanks for this change @wparad! It looks very useful, and I agree with you that the X-Ray SDK should not throw exceptions to the user's app after the initial initialization stage. This is the same philosophy follows with the OTel JS Contrib project which supports X-Ray :)

Would you have bandwidth to fix the tests so we can get this merged?

@wparad wparad requested a review from a team as a code owner May 27, 2022 18:23
@codecov-commenter
Copy link

Codecov Report

Merging #467 (99b25dd) into master (7283182) will decrease coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
- Coverage   82.74%   82.58%   -0.16%     
==========================================
  Files          36       36              
  Lines        1744     1751       +7     
==========================================
+ Hits         1443     1446       +3     
- Misses        301      305       +4     
Impacted Files Coverage Δ
lib/segments/segment.js 81.13% <0.00%> (-0.92%) ⬇️
lib/segments/attributes/subsegment.js 78.80% <0.00%> (-0.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7283182...99b25dd. Read the comment docs.

@wparad
Copy link
Contributor Author

wparad commented May 27, 2022

@NathanielRN done!

@willarmiros
Copy link
Contributor

Please note that my comments still apply as well :)

@wparad
Copy link
Contributor Author

wparad commented May 27, 2022

Please note that my comments still apply as well :)

@willarmiros, there are no comments on this PR that I can see.

@willarmiros
Copy link
Contributor

Ah! That's because they were still pending :)

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@willarmiros willarmiros merged commit 34d8126 into aws:master Jun 12, 2022
@wparad wparad deleted the stop-unnecessary-throw-error branch June 12, 2022 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants