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

ref: Rename APM tracking feature flags to tracing #2450

Merged
merged 6 commits into from
Nov 28, 2022

Conversation

kevinrenskers
Copy link
Contributor

📜 Description

I've only done a simple rename now, no backwards compatible duplicate properties that forward their value to the new properties. If we're doing a major release anyway, I am not sure if that is really worth the trouble and extra code to maintain. Most users will use the default flags, and for the others Xcode will suggest the proper new spelling with a "fix it" button doing the work.

💡 Motivation and Context

Closes #2417

💚 How did you test it?

Everything still runs as before, green tests.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@kevinrenskers kevinrenskers changed the base branch from master to 8.0.0 November 28, 2022 09:24
@github-actions
Copy link

github-actions bot commented Nov 28, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1226.45 ms 1248.20 ms 21.75 ms
Size 20.75 KiB 383.68 KiB 362.92 KiB

Baseline results on branch: 8.0.0

Startup times

Revision Plain With Sentry Diff
1ce879f 1258.12 ms 1260.90 ms 2.78 ms
f91714d 1222.06 ms 1247.00 ms 24.94 ms
7eee302 1228.73 ms 1241.94 ms 13.21 ms
dcac8ad 1238.82 ms 1247.80 ms 8.98 ms
2ae7db9 1231.37 ms 1239.98 ms 8.61 ms
d10145a 1232.65 ms 1257.55 ms 24.90 ms
68094b3 1214.14 ms 1255.09 ms 40.95 ms
e2cec76 1189.48 ms 1229.84 ms 40.36 ms
c9129b6 1231.86 ms 1270.11 ms 38.25 ms
bbe81cb 1257.25 ms 1272.24 ms 14.99 ms

App size

Revision Plain With Sentry Diff
1ce879f 20.75 KiB 381.81 KiB 361.06 KiB
f91714d 20.75 KiB 381.87 KiB 361.12 KiB
7eee302 20.75 KiB 374.73 KiB 353.97 KiB
dcac8ad 20.75 KiB 379.11 KiB 358.36 KiB
2ae7db9 20.75 KiB 381.87 KiB 361.12 KiB
d10145a 20.75 KiB 379.12 KiB 358.36 KiB
68094b3 20.75 KiB 373.94 KiB 353.19 KiB
e2cec76 20.75 KiB 381.81 KiB 361.06 KiB
c9129b6 20.75 KiB 381.81 KiB 361.06 KiB
bbe81cb 20.75 KiB 381.81 KiB 361.06 KiB

Previous results on branch: ref/2417-tracking-to-tracing

Startup times

Revision Plain With Sentry Diff
93a8795 1239.80 ms 1271.24 ms 31.44 ms

App size

Revision Plain With Sentry Diff
93a8795 20.75 KiB 383.68 KiB 362.93 KiB

@kevinrenskers
Copy link
Contributor Author

I don't understand why the Integration Tests / Integration Web Requests workflow keeps failing with a LOT of network test failures. Is there something wrong with the Alamofire patch file?

@kevinrenskers
Copy link
Contributor Author

Never mind, it fails in all recent PRs, so it's not related to the changes in this PR. That's a relief, but the question is still why it fails everywhere of course.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM. Yes, it's fine to not deprecate it. You can easily fix the breaking change, and you should be calling these flags only once or at most a couple of times in your code base.

@philipphofmann
Copy link
Member

I don't understand why the Integration Tests / Integration Web Requests workflow keeps failing with a LOT of network test failures. Is there something wrong with the Alamofire patch file?

Could you maybe look into why they are failing @kevinrenskers? They are also failing on master.

@kevinrenskers
Copy link
Contributor Author

If it's also broken on master then I'll just merge this PR with the one failing check. How can I run this test locally?

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.

Rename APM tracking feature flags to tracing
3 participants