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

Remove use alias for instrument #3403

Merged
merged 2 commits into from
Feb 8, 2024
Merged

Conversation

TonyCTHsu
Copy link
Contributor

2.0 Upgrade Guide notes

🚨 Breaking change: Remove use method for configuration, replace with instrument.

For example

Datadog.configure do |c|
  c.tracing.use :mysql2
end

to

Datadog.configure do |c|
  c.tracing.instrument :mysql2
end

@TonyCTHsu TonyCTHsu requested a review from a team as a code owner January 25, 2024 19:53
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Jan 25, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d67d0a4) 98.11% compared to head (889182b) 98.12%.
Report is 34 commits behind head on 2.0.

Additional details and impacted files
@@            Coverage Diff             @@
##              2.0    #3403      +/-   ##
==========================================
+ Coverage   98.11%   98.12%   +0.01%     
==========================================
  Files        1250     1243       -7     
  Lines       72401    72013     -388     
  Branches     3391     3376      -15     
==========================================
- Hits        71033    70661     -372     
+ Misses       1368     1352      -16     

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

@ivoanjo
Copy link
Member

ivoanjo commented Jan 26, 2024

Can we consider leaving this one in? The maintenance cost seems to be very low -- it's one line, and I'm wondering if having this one line is the difference between a lot of customers smooth no-changes-needed upgrade from 1.x to 2.x?

So it seems like a nice win, even if it's slightly "ugly" to keep both.

@TonyCTHsu
Copy link
Contributor Author

Can we consider leaving this one in?

@ivoanjo , I believe the alias was introduced for backward compatibility at 1.x for 0.x. and I think it is fair to consider keeping this considered the maintenance cost is low. However, I prefer to remove it because I believe it is also fairly easy to change them in user's configuration.

@ivoanjo
Copy link
Member

ivoanjo commented Jan 26, 2024

I'll say that in my opinion no changes > easy changes ;)

@p-datadog
Copy link
Contributor

If people actually use use, it seems that it's removed with no deprecation period leading to errors when these users upgrade 1.0 -> 2.0. Perhaps adding a deprecation warning to master branch advising users to replace use with instrument would be good?

@ekump
Copy link
Contributor

ekump commented Feb 1, 2024

I'm in favor of moving forward with this change. It may seem insignificant, but it does make our public interface and docs less confusing.

If people actually use use, it seems that it's removed with no deprecation period leading to errors when these users upgrade 1.0 -> 2.0

Users have to opt-in to a major release and will need to follow the upgrade guide. This change, along with all the other breaking changes shouldn't really surprise anyone. That being said, we should reasonably do what we can to ease the transition.

Perhaps adding a deprecation warning to master branch advising users to replace use with instrument would be good?

If it is relatively easy to tell if a customer is using the alias, I think this is a good idea.

@TonyCTHsu
Copy link
Contributor Author

PR for the logging deprecation warning: #3438

@TonyCTHsu TonyCTHsu self-assigned this Feb 6, 2024
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/remove-use-alias-instrument branch from 889182b to 4d40b2f Compare February 6, 2024 20:28
@TonyCTHsu TonyCTHsu requested a review from a team as a code owner February 6, 2024 20:54
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/remove-use-alias-instrument branch from 7bd0ddc to 73902cd Compare February 8, 2024 09:00
@TonyCTHsu TonyCTHsu merged commit e609bdb into 2.0 Feb 8, 2024
28 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/remove-use-alias-instrument branch February 8, 2024 10:51
@TonyCTHsu TonyCTHsu added this to the 2.0 milestone Feb 20, 2024
@ivoanjo ivoanjo added the 2.0 label Mar 14, 2024
@TonyCTHsu TonyCTHsu modified the milestones: 2.0, 2.0.0.beta1 Mar 22, 2024
@TonyCTHsu TonyCTHsu mentioned this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants