Skip to content

Updated ProfileMeasurementValue types#2412

Merged
stefanosiano merged 4 commits into
mainfrom
fix/profile-measurement-types
Dec 6, 2022
Merged

Updated ProfileMeasurementValue types#2412
stefanosiano merged 4 commits into
mainfrom
fix/profile-measurement-types

Conversation

@stefanosiano

@stefanosiano stefanosiano commented Dec 2, 2022

Copy link
Copy Markdown
Contributor

📜 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

github-actions Bot commented Dec 2, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Dec 2, 2022 •
edited
Loading

Copy link
Copy Markdown

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
markushi
markushi approved these changes Dec 6, 2022
View reviewed changes

@markushi markushi left a comment

Copy link
Copy Markdown
Member

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

stefanosiano commented Dec 6, 2022

Copy link
Copy Markdown
Contributor 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

Reviewers

@markushi markushi markushi approved these changes
@adinauer adinauer Awaiting requested review from adinauer adinauer is a code owner
@romtsn romtsn Awaiting requested review from romtsn romtsn is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stefanosiano @codecov-commenter @markushi