-
-
Notifications
You must be signed in to change notification settings - Fork 354
feat(metrics): Updates Metrics code to correctly process envelops withapplication/vnd.sentry.items.trace-metric+json content type
#5440
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
Conversation
|
application/vnd.sentry.items.trace-metric+json content type
antonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @alwx 🚀
I've retested with the sample app and was able to see metrics on the dashboard :)
Added a comment on a linter issue but other than that LGTM.
Let's also investigate @lucas-zimerman comment before merging 🙇
|
@sentry review |
| bytesPayload = itemPayload; | ||
| } else { | ||
| bytesContentType = 'application/vnd.sentry.items.log+json'; | ||
| bytesContentType = typeof itemHeader.content_type === 'string' ? itemHeader.content_type : 'application/json'; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! once we fix the linting/build issues, we could merge the PR :D
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d1bfbde+dirty | 478.88 ms | 505.52 ms | 26.64 ms |
| 93137d1+dirty | 400.15 ms | 424.74 ms | 24.59 ms |
| 05bef0e+dirty | 349.78 ms | 334.04 ms | -15.74 ms |
| 77061ed+dirty | 369.55 ms | 408.35 ms | 38.80 ms |
| a941c72+dirty | 489.85 ms | 549.17 ms | 59.32 ms |
| 6479fd5+dirty | 412.95 ms | 434.02 ms | 21.07 ms |
| 07808fb+dirty | 419.10 ms | 419.08 ms | -0.02 ms |
| b80b14f+dirty | 505.06 ms | 534.32 ms | 29.26 ms |
| 90afdd3+dirty | 375.94 ms | 377.52 ms | 1.58 ms |
| 5526494 | 440.84 ms | 448.36 ms | 7.52 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| d1bfbde+dirty | 43.75 MiB | 47.99 MiB | 4.24 MiB |
| 93137d1+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 05bef0e+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 77061ed+dirty | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| a941c72+dirty | 43.75 MiB | 48.05 MiB | 4.29 MiB |
| 6479fd5+dirty | 17.75 MiB | 19.68 MiB | 1.94 MiB |
| 07808fb+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| b80b14f+dirty | 43.75 MiB | 48.04 MiB | 4.29 MiB |
| 90afdd3+dirty | 17.75 MiB | 19.70 MiB | 1.95 MiB |
| 5526494 | 17.75 MiB | 19.68 MiB | 1.93 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 88890fe+dirty | 328.30 ms | 319.85 ms | -8.45 ms |
| c1573b3+dirty | 355.65 ms | 448.82 ms | 93.17 ms |
| 2104bb9+dirty | 313.00 ms | 309.76 ms | -3.24 ms |
| 1d62dde+dirty | 366.59 ms | 408.80 ms | 42.21 ms |
| 8a4ce6f+dirty | 401.11 ms | 381.92 ms | -19.19 ms |
| c4e097a+dirty | 382.43 ms | 443.77 ms | 61.34 ms |
| 46da307+dirty | 356.62 ms | 415.02 ms | 58.40 ms |
| 8e653ac+dirty | 304.49 ms | 308.84 ms | 4.35 ms |
| e2fa43d+dirty | 326.56 ms | 372.88 ms | 46.32 ms |
| 07808fb+dirty | 392.47 ms | 451.94 ms | 59.47 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 88890fe+dirty | 7.15 MiB | 8.44 MiB | 1.28 MiB |
| c1573b3+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 2104bb9+dirty | 7.15 MiB | 8.46 MiB | 1.30 MiB |
| 1d62dde+dirty | 7.15 MiB | 8.46 MiB | 1.31 MiB |
| 8a4ce6f+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| c4e097a+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 46da307+dirty | 7.15 MiB | 8.41 MiB | 1.26 MiB |
| 8e653ac+dirty | 7.15 MiB | 8.46 MiB | 1.31 MiB |
| e2fa43d+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
| 07808fb+dirty | 7.15 MiB | 8.43 MiB | 1.28 MiB |
|
@lucas-zimerman that's done now |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 083f560+dirty | 1215.27 ms | 1231.96 ms | 16.69 ms |
| f3b058c+dirty | 1221.43 ms | 1219.85 ms | -1.58 ms |
| 23080e5+dirty | 1221.39 ms | 1222.08 ms | 0.70 ms |
| 534ba8c+dirty | 1225.00 ms | 1237.43 ms | 12.43 ms |
| 493c1a1+dirty | 1220.50 ms | 1221.30 ms | 0.80 ms |
| 6416d6c+dirty | 1222.83 ms | 1222.04 ms | -0.79 ms |
| 46da307+dirty | 1213.45 ms | 1207.96 ms | -5.49 ms |
| e07935d+dirty | 1225.85 ms | 1227.72 ms | 1.87 ms |
| ec14be7+dirty | 1229.62 ms | 1230.53 ms | 0.91 ms |
| 90afdd3+dirty | 1216.17 ms | 1225.55 ms | 9.38 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 083f560+dirty | 3.19 MiB | 4.56 MiB | 1.38 MiB |
| f3b058c+dirty | 3.41 MiB | 4.67 MiB | 1.25 MiB |
| 23080e5+dirty | 3.19 MiB | 4.48 MiB | 1.29 MiB |
| 534ba8c+dirty | 3.19 MiB | 4.38 MiB | 1.19 MiB |
| 493c1a1+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 6416d6c+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| 46da307+dirty | 3.19 MiB | 4.44 MiB | 1.25 MiB |
| e07935d+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| ec14be7+dirty | 3.19 MiB | 4.54 MiB | 1.36 MiB |
| 90afdd3+dirty | 3.19 MiB | 4.55 MiB | 1.37 MiB |
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 083f560+dirty | 1227.33 ms | 1242.02 ms | 14.69 ms |
| f3b058c+dirty | 1221.30 ms | 1214.62 ms | -6.68 ms |
| 23080e5+dirty | 1216.02 ms | 1224.94 ms | 8.91 ms |
| 534ba8c+dirty | 1230.22 ms | 1231.18 ms | 0.96 ms |
| 493c1a1+dirty | 1207.58 ms | 1211.80 ms | 4.22 ms |
| 6416d6c+dirty | 1220.38 ms | 1222.98 ms | 2.60 ms |
| 46da307+dirty | 1217.08 ms | 1224.16 ms | 7.08 ms |
| e07935d+dirty | 1217.37 ms | 1211.44 ms | -5.93 ms |
| ec14be7+dirty | 1234.64 ms | 1245.54 ms | 10.90 ms |
| 90afdd3+dirty | 1233.90 ms | 1240.90 ms | 7.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 083f560+dirty | 2.63 MiB | 4.00 MiB | 1.36 MiB |
| f3b058c+dirty | 3.41 MiB | 4.67 MiB | 1.25 MiB |
| 23080e5+dirty | 2.63 MiB | 3.91 MiB | 1.28 MiB |
| 534ba8c+dirty | 2.63 MiB | 3.81 MiB | 1.18 MiB |
| 493c1a1+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| 6416d6c+dirty | 3.41 MiB | 4.59 MiB | 1.18 MiB |
| 46da307+dirty | 2.63 MiB | 3.87 MiB | 1.24 MiB |
| e07935d+dirty | 3.41 MiB | 4.58 MiB | 1.17 MiB |
| ec14be7+dirty | 2.63 MiB | 3.98 MiB | 1.34 MiB |
| 90afdd3+dirty | 2.63 MiB | 3.99 MiB | 1.35 MiB |
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving as request changes for a second check.
@antonis @alwx, Could you double-check that metrics are being sent on Android? I have tried this branch today on an Android simulator, and despite seeing it on spotlightJs, I don't see it on sentry.io (I was able to see if I disabled the native integration)
EDIT: If you don't see it on Android, we may need to alter the Java SDK because it's overwriting the envelope type to unknown
|
@lucas-zimerman you seem to be correct — the enabled native integration kills it. But as I understood, it seems to be related to unknown spans on Android, which means it requires no changes to RN code and this PR in particular, right? |
@alwx I think this is the case 👍 Maybe we can update the changelog in this (or a separate PR) clarifying that the metrics are released in beta only for iOS and merge/release this fix. |
|
@antonis makes sense, I'd do that. @lucas-zimerman what do you think? |
|
I also agree. EDIT: There is no additional changes required on this PR, should we wait for the Android update so both iOS/Android have support for Metrics, or release this PR mentioning that it's only supported on iOS? |
Since it is a beta release I think it would be ok to release only on iOS. I'll leave that up to @alwx 🙇 |
antonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
|
@lucas-zimerman @antonis I've updated the changelog entry with information that it only works on iOS. |
lucas-zimerman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
📢 Type of change
📜 Description
Updates metrics to be processed correctly.
In addition to that, updates some of the tests that were incorrectly using content type for logs.
💚 How did you test it?
Tests, tests, tests. And just running the app and seing myself.
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps
#skip-changelog