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

Updated ProfileMeasurementValue types #2412

Merged
merged 4 commits into from
Dec 6, 2022

Conversation

stefanosiano
Copy link
Member

📜 Description

Changed ProfileMeasurementValue types: timestamp is now a string and value is a double
Added a check against negative timestamps

💡 Motivation and Context

Relay added a stronger type check on the profile measurements. This caused profiles to be rejected. While the issue has already been addressed in relay, it's better to update the schema to what relay expects.

💚 How did you test it?

Updated unit test

📝 Checklist

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

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 345.06 ms 386.94 ms 41.88 ms
Size 1.73 MiB 2.32 MiB 612.44 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
31f3e4c 325.22 ms 342.77 ms 17.55 ms
81a1a6c 328.73 ms 421.28 ms 92.55 ms
7597ded 289.60 ms 339.69 ms 50.09 ms
507f924 342.51 ms 402.65 ms 60.14 ms
38e4f11 358.20 ms 433.73 ms 75.53 ms
3695453 301.78 ms 371.14 ms 69.36 ms
81a1a6c 294.04 ms 341.19 ms 47.15 ms
4a9c176 319.77 ms 363.20 ms 43.43 ms
16371c5 314.02 ms 394.54 ms 80.52 ms
f809aac 301.51 ms 346.60 ms 45.09 ms

App size

Revision Plain With Sentry Diff
31f3e4c 1.73 MiB 2.32 MiB 612.47 KiB
81a1a6c 1.73 MiB 2.32 MiB 612.47 KiB
7597ded 1.73 MiB 2.32 MiB 609.88 KiB
507f924 1.73 MiB 2.32 MiB 609.95 KiB
38e4f11 1.73 MiB 2.32 MiB 609.82 KiB
3695453 1.73 MiB 2.32 MiB 611.62 KiB
81a1a6c 1.73 MiB 2.32 MiB 612.47 KiB
4a9c176 1.73 MiB 2.33 MiB 612.69 KiB
16371c5 1.73 MiB 2.32 MiB 611.62 KiB
f809aac 1.73 MiB 2.32 MiB 608.63 KiB

Previous results on branch: fix/profile-measurement-types

Startup times

Revision Plain With Sentry Diff
4abc1c0 355.67 ms 368.35 ms 12.68 ms
a190218 323.27 ms 384.22 ms 60.96 ms

App size

Revision Plain With Sentry Diff
4abc1c0 1.73 MiB 2.32 MiB 612.47 KiB
a190218 1.73 MiB 2.32 MiB 612.47 KiB

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Base: 80.03% // Head: 80.04% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (97634f7) compared to base (f122116).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2412   +/-   ##
=========================================
  Coverage     80.03%   80.04%           
  Complexity     3765     3765           
=========================================
  Files           301      301           
  Lines         14207    14206    -1     
  Branches       1884     1883    -1     
=========================================
  Hits          11371    11371           
  Misses         2092     2092           
+ Partials        744      743    -1     
Impacted Files Coverage Δ
...y/profilemeasurements/ProfileMeasurementValue.java 59.57% <100.00%> (+1.24%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stefanosiano stefanosiano marked this pull request as ready for review December 5, 2022 16:34
Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

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

Looks good to me! Do we need to migrate other components (e.g. hybrid SDKs) or wait for a backend release (e.g. on relay) before this can be merged?

@stefanosiano
Copy link
Member Author

Looks good to me! Do we need to migrate other components (e.g. hybrid SDKs) or wait for a backend release (e.g. on relay) before this can be merged?

nope: this was the expected format from the beginning, but relay was previously more "permissive" and profiles were ingested.
This change is because relay was updated to check strictly the types, but that check was temporary reverted to allow the sdk to update the code

@stefanosiano stefanosiano merged commit 20d7d3d into main Dec 6, 2022
@stefanosiano stefanosiano deleted the fix/profile-measurement-types branch December 6, 2022 11:16
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