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

fix(errors): revert breaking change #3796

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

mabdinur
Copy link
Contributor

What does this PR do?

#3776 renamed the Error.set_error method to set_error_tags. Renaming a method in the public api is a breaking change. This PR reverts this breaking change and adds deprecation warning.

Motivation:

Additional Notes:

How to test the change?

Unsure? Have a question? Request a review!

@mabdinur mabdinur requested a review from a team as a code owner July 19, 2024 14:59
@mabdinur mabdinur force-pushed the munir/revert-api-breaking-change branch from 2f6823b to dcda9d0 Compare July 19, 2024 15:01
@pr-commenter
Copy link

pr-commenter bot commented Jul 19, 2024

Benchmarks

Benchmark execution time: 2024-07-19 15:11:43

Comparing candidate commit dcda9d0 in PR branch munir/revert-api-breaking-change with baseline commit 2f089cd in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 2 unstable metrics.

scenario:Tracing.log_correlation

  • 🟩 throughput [+2637.583op/s; +2886.706op/s] or [+2.302%; +2.520%]

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 12.50000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 97.89%. Comparing base (2f089cd) to head (dcda9d0).

Files Patch % Lines
spec/datadog/tracing/metadata/errors_spec.rb 8.33% 11 Missing ⚠️
lib/datadog/tracing/metadata/errors.rb 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3796      +/-   ##
==========================================
- Coverage   97.91%   97.89%   -0.03%     
==========================================
  Files        1248     1248              
  Lines       75192    75197       +5     
  Branches     3638     3638              
==========================================
- Hits        73628    73615      -13     
- Misses       1564     1582      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +14 to +17
Datadog::Core.log_deprecation do
'Errors.set_error(..) is deprecated. ' \
'Use Errors.set_error_tags(..) instead.'
end
Copy link
Member

Choose a reason for hiding this comment

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

Why is set_error deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors.set_error shadows SpanOperation.set_error. SpanOperation.set_error always sets the status on a Span to error. In OpenTelemetry we need to decouple setting error tags from setting error statuses. To do this I renamed Errors.set_error to Errors.set_error_tags in a previous PR. This was a breaking change. Here I am adding set_error_tags back but deprecating it to avoid maintaining two methods that do the exact same thing.

@mabdinur mabdinur requested a review from marcotc July 22, 2024 14:09
@TonyCTHsu TonyCTHsu added this to the 2.3.0 milestone Jul 25, 2024
@marcotc marcotc merged commit 47f12db into master Jul 29, 2024
171 checks passed
@marcotc marcotc deleted the munir/revert-api-breaking-change branch July 29, 2024 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants