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

Revert "feat(api): add attributes argument to recordException API (#4071)" #4137

Merged
merged 3 commits into from
Sep 11, 2023

Conversation

pichlermarc
Copy link
Member

Which problem is this PR solving?

This reverts commit cd4998a.
Rolls back the breaking change introduced via https://github.com/open-telemetry/opentelemetry-js/releases/tag/api%2Fv1.5.0

I'll open a release PR for API 1.6.0 once this is merged.

Fixes #4136

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #4137 (c5ba7dd) into main (5fcd8cf) will increase coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head c5ba7dd differs from pull request most recent head 256857a. Consider uploading reports for the commit 256857a to get more accurate results

@@           Coverage Diff           @@
##             main    #4137   +/-   ##
=======================================
  Coverage   92.24%   92.24%           
=======================================
  Files         326      326           
  Lines        9305     9299    -6     
  Branches     1974     1971    -3     
=======================================
- Hits         8583     8578    -5     
+ Misses        722      721    -1     
Files Changed Coverage
api/src/trace/NonRecordingSpan.ts 0.00%
packages/opentelemetry-sdk-trace-base/src/Span.ts 100.00%

@pichlermarc pichlermarc marked this pull request as ready for review September 11, 2023 14:45
@pichlermarc pichlermarc requested a review from a team September 11, 2023 14:45
@pichlermarc
Copy link
Member Author

cc @open-telemetry/javascript-approvers @open-telemetry/javascript-maintainers looks like #4071 was breaking (see #4136)

@pichlermarc pichlermarc merged commit 27897d6 into open-telemetry:main Sep 11, 2023
@pichlermarc pichlermarc deleted the fix/4136 branch September 11, 2023 15:36
@dvoytenko
Copy link
Contributor

What's the resolution on this? Does it mean that no modification for an existing API is possible, even when it's required by the spec? Or should the downstream dependencies be able to adjust to the API changes?

Also, not to complain, but IMHO it'd be helpful to ping the original pull request when it's getting reverted.

@pichlermarc
Copy link
Member Author

What's the resolution on this? Does it mean that no modification for an existing API is possible, even when it's required by the spec? Or should the downstream dependencies be able to adjust to the API changes?

Downstream dependencies will likely need to adjust. The spec is clarifying it here: open-telemetry/opentelemetry-specification#3695. Once the spec PR is merged, we'll proceed accordingly.

Also, not to complain, but IMHO it'd be helpful to ping the original pull request when it's getting reverted.

Yep, this must've slipped through. Sorry about that.

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.

Changes on 9-11-23 break dd-trace
3 participants