-
Notifications
You must be signed in to change notification settings - Fork 812
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(api): add attributes argument to recordException API #4071
feat(api): add attributes argument to recordException API #4071
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4071 +/- ##
==========================================
- Coverage 90.52% 89.74% -0.78%
==========================================
Files 159 140 -19
Lines 3757 3014 -743
Branches 835 653 -182
==========================================
- Hits 3401 2705 -696
+ Misses 356 309 -47
|
For reference, open-telemetry/opentelemetry-js-api#97 is the last attempt to implement the feature. @dyladan would you mind confirming if the condition is changed since then (about the comment open-telemetry/opentelemetry-js-api#97 (comment))? |
That change indeed looks very similar. Not sure why it didn't fully go in. It looks like the |
We've had several feature releases of the API since then. There is no need to backport fixes to previous minor versions. The backport requirement is only for major versions according to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/versioning-and-stability.md#api-support |
cb79dc3
to
55e1b86
Compare
I updated the changelog to unblock the changelog workflow. |
55e1b86
to
ec0e1e1
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.
LGTM
@legendecas @dyladan thank you for your reviews! How do I merge this PR now? |
ec0e1e1
to
8f71f2c
Compare
@MSNev I update the APIs as requested. RE:
Could you point me to where I need to make this change? |
I believe you have done this well in the latest commit. |
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.
Still LGTM.
…en-telemetry#4071)" This reverts commit cd4998a.
Which problem is this PR solving?
Adds the optional
attributes
argument to therecordException
API.Fixes #4070
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: