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

deprecated root interfaces for old metrics #619

Merged
merged 2 commits into from
Oct 28, 2024
Merged

deprecated root interfaces for old metrics #619

merged 2 commits into from
Oct 28, 2024

Conversation

bryce-b
Copy link
Member

@bryce-b bryce-b commented Oct 25, 2024

I chose to only deprecate the root interfaces for metrics, rather than all the various meters

@nachoBonafonte
Copy link
Member

nachoBonafonte commented Oct 26, 2024

How about directly renaming StableMetric -> Metric and existing Metric to OldMetrics, I know it can sound painful at first but it may be quickly solved by the users of the old metrics by using something like typealias Metric = OldMetrics. It may seem a bit drastic, but is imho the fastest way to end up using Metric as the standard name, instead of carrying the Stable label forever.

@bryce-b
Copy link
Member Author

bryce-b commented Oct 28, 2024

I thought about this, but I'm worried that marking the old metrics deprecated and also changing their names will result in some confusion, and will result in users having their APIs change in some classes without them realizing. For example, the StableMetrics StableMeter will be renamed to Meter and the "old metrics" Meter will be marked as deprecated and renamed OldMeter. This will result in no deprecation warning for anyone using Meter, but it will also require immediate intervention from users when they receive the change. I think it would be better if we warn people the change is coming with the deprecation notice initially, but not force immediate interaction with code-fixing and give users some time to switch over to the stable version, and do a name change later (which will be less confusing and easier to do with developer tools).

@nachoBonafonte, What do you think?

@nachoBonafonte
Copy link
Member

You are right, a warning period is necessary here

@bryce-b bryce-b merged commit 816c73e into main Oct 28, 2024
11 checks passed
Copy link
Contributor

@vvydier vvydier left a comment

Choose a reason for hiding this comment

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

good plan.

@bryce-b bryce-b deleted the deprecateMetrics branch October 29, 2024 17:34
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.

3 participants