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

change(profiling): deprecate old trace-and continuous profiling API #4854

Draft
wants to merge 3 commits into
base: armcknight/profiling/new-continuous-apis/0-topic-branch
Choose a base branch
from

Conversation

armcknight
Copy link
Member

For #4852. Deprecate all old API. We'll be adding new API that improve the way continuous profiling works in future PRs (see the epic issue #4851).

#skip-changelog

@armcknight
Copy link
Member Author

armcknight commented Feb 14, 2025

Because there is no way to ignore deprecation warnings in Swift, and we have this Swift implementation that references one of the things I need to deprecate here, I am now faced with a decision to make on what kind of hack to implement to work around this limitation to resolve it, because it's breaking basically every single CI job we run:
image

ETA: i'm temporarily disabling warnings as errors until i can get a workaround in place, so it doesn't slow down further work on the rest of the tasks.

Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.250%. Comparing base (a286774) to head (3b04d2d).

Additional details and impacted files

Impacted file tree graph

@@                                      Coverage Diff                                      @@
##           armcknight/profiling/new-continuous-apis/0-topic-branch     #4854       +/-   ##
=============================================================================================
+ Coverage                                                   92.165%   92.250%   +0.084%     
=============================================================================================
  Files                                                          658       658               
  Lines                                                        77223     77229        +6     
  Branches                                                     27169     27951      +782     
=============================================================================================
+ Hits                                                         71173     71244       +71     
+ Misses                                                        5955      5886       -69     
- Partials                                                        95        99        +4     

see 23 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a286774...3b04d2d. Read the comment docs.

Copy link

github-actions bot commented Feb 14, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1212.30 ms 1241.04 ms 28.74 ms
Size 22.32 KiB 819.55 KiB 797.24 KiB

Previous results on branch: armcknight/profiling/new-continuous-apis/1-deprecations

Startup times

Revision Plain With Sentry Diff
cee8a33 1229.81 ms 1250.15 ms 20.34 ms

App size

Revision Plain With Sentry Diff
cee8a33 22.32 KiB 819.56 KiB 797.24 KiB

@philipphofmann
Copy link
Member

philipphofmann commented Feb 17, 2025

Because there is no way to ignore deprecation warnings in Swift, and we have this Swift implementation that references one of the things I need to deprecate here, I am now faced with a decision to make on what kind of hack to implement to work around this limitation to resolve it, because it's breaking basically every single CI job we run:

ETA: i'm temporarily disabling warnings as errors until i can get a workaround in place, so it doesn't slow down further work on the rest of the tasks.

Yeah, that sucks pretty much. You can mark the whole method as deprecated like this:

@available(*, deprecated, message: """
This method is only deprecated to silence the deprecation warning of the property \
segment. Our Xcode project has deprecations as warnings and warnings as errors \
configured. Therefore, compilation fails without marking this init method as \
deprecated. It is safe to use this deprecated init method. Instead of turning off \
deprecation warnings for the whole project, we accept the tradeoff of marking this \
init method as deprecated because we don't expect many users to use it. Sadly, \
Swift doesn't offer a better way of silencing a deprecation warning.
""")

I'm starting to believe that we should disable deprecation warnings or disable change warnings as errors and set every warning to an error level that should be an error.

…w to deal with the deprecated usage in SentryEnabledFeaturesBuilder
@armcknight armcknight force-pushed the armcknight/profiling/new-continuous-apis/1-deprecations branch from 2888371 to 3b04d2d Compare February 18, 2025 07:04
Comment on lines +164 to +165
GCC_TREAT_WARNINGS_AS_ERRORS = NO
SWIFT_TREAT_WARNINGS_AS_ERRORS = NO
Copy link
Member

Choose a reason for hiding this comment

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

h: Ah 😡 , this is again a point where Swift sucks pretty much compared to ObjC. I didn't find a way to only treat deprecations as warnings or properly ignore deprecation warnings. I thought it's possible but I think it's not. The problem is that we open the door to ignore new compiler warnings, which could cause bugs. I enabled treat warnings as errors some time ago because a warning led to a severe bug, but I can't recall which one.

If we disable warnings as errors, we need to ensure we're not introducing new warnings by accident. We could have an allowed warning list in a file and then compare it with the Xcode output or something. This should run in CI and fail if there's a new warning that's not on that list, in my opinion. We shouldn't do this in this PR thought. Maybe using the weird deprecated workaround for once more is acdeptable.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, i'll try the deprecation workaround you showed me. this change turning off warnings as errors isn't meant to be merged.

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.

2 participants