Fixes the issue with unit mismatch in adjustTransactionDuration#5740
Fixes the issue with unit mismatch in adjustTransactionDuration#5740
adjustTransactionDuration#5740Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ea3e26e+dirty | 1229.13 ms | 1228.46 ms | -0.67 ms |
| 80e4616+dirty | 1221.32 ms | 1225.64 ms | 4.32 ms |
| 818a608+dirty | 1205.76 ms | 1208.00 ms | 2.24 ms |
| 77061ed+dirty | 1233.16 ms | 1234.88 ms | 1.71 ms |
| bef3709+dirty | 1222.07 ms | 1220.24 ms | -1.83 ms |
| a206511+dirty | 1185.00 ms | 1186.35 ms | 1.35 ms |
| 74979ac+dirty | 1210.49 ms | 1213.31 ms | 2.82 ms |
| a2bb688+dirty | 1223.53 ms | 1232.90 ms | 9.37 ms |
| 8a868fe+dirty | 1221.50 ms | 1230.78 ms | 9.28 ms |
| d590428+dirty | 1211.77 ms | 1220.51 ms | 8.75 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ea3e26e+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 80e4616+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| 818a608+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| 77061ed+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| bef3709+dirty | 3.38 MiB | 4.78 MiB | 1.40 MiB |
| a206511+dirty | 3.41 MiB | 4.67 MiB | 1.25 MiB |
| 74979ac+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| a2bb688+dirty | 2.63 MiB | 3.99 MiB | 1.36 MiB |
| 8a868fe+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| d590428+dirty | 3.38 MiB | 4.78 MiB | 1.39 MiB |
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86584b7+dirty | 463.83 ms | 500.31 ms | 36.48 ms |
| 9a81842+dirty | 412.23 ms | 416.56 ms | 4.33 ms |
| c637fc7+dirty | 433.70 ms | 467.76 ms | 34.06 ms |
| d73150f+dirty | 411.21 ms | 465.86 ms | 54.65 ms |
| fa7bb7e+dirty | 350.37 ms | 377.02 ms | 26.65 ms |
| 3bd3f0d+dirty | 447.21 ms | 472.31 ms | 25.10 ms |
| 88890fe+dirty | 350.94 ms | 365.74 ms | 14.80 ms |
| 95aaf8a | 437.89 ms | 419.45 ms | -18.44 ms |
| c0842e7+dirty | 527.76 ms | 566.69 ms | 38.93 ms |
| 1e7a472+dirty | 348.80 ms | 362.55 ms | 13.75 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 86584b7+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 9a81842+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| c637fc7+dirty | 43.75 MiB | 48.40 MiB | 4.64 MiB |
| d73150f+dirty | 43.75 MiB | 48.55 MiB | 4.80 MiB |
| fa7bb7e+dirty | 17.75 MiB | 19.75 MiB | 2.00 MiB |
| 3bd3f0d+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 88890fe+dirty | 17.75 MiB | 19.71 MiB | 1.96 MiB |
| 95aaf8a | 17.75 MiB | 19.68 MiB | 1.93 MiB |
| c0842e7+dirty | 43.75 MiB | 48.41 MiB | 4.66 MiB |
| 1e7a472+dirty | 17.75 MiB | 19.70 MiB | 1.96 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ea3e26e+dirty | 1216.61 ms | 1214.15 ms | -2.47 ms |
| 80e4616+dirty | 1206.90 ms | 1205.94 ms | -0.96 ms |
| 818a608+dirty | 1218.84 ms | 1223.18 ms | 4.34 ms |
| 77061ed+dirty | 1210.77 ms | 1218.45 ms | 7.68 ms |
| bef3709+dirty | 1217.79 ms | 1225.33 ms | 7.54 ms |
| a206511+dirty | 1225.02 ms | 1223.74 ms | -1.28 ms |
| 74979ac+dirty | 1212.33 ms | 1212.54 ms | 0.21 ms |
| a2bb688+dirty | 1244.82 ms | 1238.60 ms | -6.22 ms |
| 8a868fe+dirty | 1206.85 ms | 1215.04 ms | 8.19 ms |
| d590428+dirty | 1221.23 ms | 1225.27 ms | 4.03 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ea3e26e+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 80e4616+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| 818a608+dirty | 3.19 MiB | 4.48 MiB | 1.29 MiB |
| 77061ed+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| bef3709+dirty | 3.38 MiB | 4.78 MiB | 1.40 MiB |
| a206511+dirty | 3.41 MiB | 4.67 MiB | 1.25 MiB |
| 74979ac+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| a2bb688+dirty | 3.19 MiB | 4.56 MiB | 1.37 MiB |
| 8a868fe+dirty | 3.38 MiB | 4.60 MiB | 1.22 MiB |
| d590428+dirty | 3.38 MiB | 4.78 MiB | 1.39 MiB |
packages/core/src/js/tracing/span.ts
Outdated
| } = { | ||
| idleTimeout: 1_000, | ||
| finalTimeout: 60_0000, | ||
| finalTimeout: 60_000, |
There was a problem hiding this comment.
h: I think this would introduce a behavioral change that may not be desired. Changing finalTimeout from 600,000 ms → 60,000 ms reduces the hard cap on navigation span lifetime from 10 minutes to 1 minute. Any app with slow screens, heavy loading, or long user sessions that previously could create valid navigation spans up to 10 minutes would now have them force-terminated at 1 minute.
Looking at the history of this I think the problem is the _ in the wrong position that was introduced with this refactoring. We should probably keep this to 10m and just fix the formatting like:
| finalTimeout: 60_000, | |
| finalTimeout: 600_000, |
Also looping in @lucas-zimerman who may have a better understanding of the history of this 🙏
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7480abe+dirty | 363.80 ms | 431.34 ms | 67.54 ms |
| 2b89ce9+dirty | 372.22 ms | 417.06 ms | 44.84 ms |
| 170d5ea+dirty | 348.79 ms | 406.94 ms | 58.15 ms |
| b1579bc+dirty | 391.87 ms | 456.26 ms | 64.39 ms |
| 73f2455+dirty | 369.33 ms | 398.90 ms | 29.57 ms |
| 0b64753+dirty | 358.55 ms | 429.16 ms | 70.61 ms |
| 6a70a7e+dirty | 382.45 ms | 424.54 ms | 42.09 ms |
| 2adbd1e+dirty | 366.13 ms | 419.49 ms | 53.36 ms |
| f8d19f8+dirty | 374.17 ms | 383.40 ms | 9.23 ms |
| 7be1f99+dirty | 369.02 ms | 399.60 ms | 30.58 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7480abe+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 2b89ce9+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 170d5ea+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| b1579bc+dirty | 43.94 MiB | 49.27 MiB | 5.33 MiB |
| 73f2455+dirty | 43.94 MiB | 48.82 MiB | 4.88 MiB |
| 0b64753+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 6a70a7e+dirty | 7.15 MiB | 8.42 MiB | 1.26 MiB |
| 2adbd1e+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
| f8d19f8+dirty | 43.94 MiB | 48.91 MiB | 4.97 MiB |
| 7be1f99+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
There was a problem hiding this comment.
There is a lint issue here. A yarn fix should correct that.
/home/admin/actions-runner/_work/sentry-react-native/sentry-react-native/packages/core/test/tracing/adjustTransactionDuration.test.ts
39:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
57:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
75:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
93:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
112:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
131:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
153:33 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
160:35 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
178:28 error This assertion is unnecessary since it does not change the type of the expression @typescript-eslint/no-unnecessary-type-assertion
📢 Type of change
📜 Description
I've suddenly found out that
spanToJSONfrom @sentry/core returns a structure that containsstart_timestamptimestamp— but the "funny" thing is that both of this timestamps are in seconds, not in milliseconds (https://github.com/getsentry/sentry-javascript/blob/develop/packages/core/src/utils/spanUtils.ts#L170).That means the way our
adjustTransactionDurationworks is wrong — thediffis in seconds but it's compared againstmaxDurationMswhich is in milliseconds. This means the safety net that should mark long-runningtransactions as
deadline_exceedednever triggers.This PR fixes that.
In addition to that, there is a small formatting issue in
span.ts— the defaultfinalTimeoutis specified as 60_0000 which should be milliseconds => needs to be 600_000 instead.📝 Checklist
sendDefaultPIIis enabled