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: Change native frames from traceId to spanId #4030

Merged

Conversation

krystofwoldrich
Copy link
Member

📢 Type of change

  • Enhancement
  • Refactoring

📜 Description

This PR changes the native frames from attaching to traceId which can be the same for multiple spans to spanId.

  • This fixes potencial situations when multiple root spans share the same trace id. This is possible since JS v8, before Transaction (now Root Spans) would always have unique trace id.
  • This is needed for attaching the native frames as span level measurements.

💚 How did you test it?

sample app, unit tests

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

@krystofwoldrich krystofwoldrich changed the title Kw/ref-native-frames-prepare-for-span-measurements ref: Change native frames from traceId to spanId Aug 16, 2024
@krystofwoldrich krystofwoldrich force-pushed the kw/ref-native-frames-prepare-for-span-measurements branch from 463da28 to 5713f87 Compare August 16, 2024 11:01
Copy link
Contributor

github-actions bot commented Aug 16, 2024

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 394.59 ms 395.65 ms 1.06 ms
Size 17.73 MiB 20.05 MiB 2.32 MiB

Copy link
Contributor

github-actions bot commented Aug 16, 2024

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1235.33 ms 1241.64 ms 6.31 ms
Size 2.36 MiB 3.07 MiB 726.55 KiB

Previous results on branch: kw/ref-native-frames-prepare-for-span-measurements

Startup times

Revision Plain With Sentry Diff
87a3e92+dirty 1229.10 ms 1223.33 ms -5.78 ms

App size

Revision Plain With Sentry Diff
87a3e92+dirty 2.36 MiB 3.07 MiB 726.56 KiB

Copy link
Contributor

github-actions bot commented Aug 16, 2024

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1224.79 ms 1219.59 ms -5.20 ms
Size 2.92 MiB 3.63 MiB 732.47 KiB

Previous results on branch: kw/ref-native-frames-prepare-for-span-measurements

Startup times

Revision Plain With Sentry Diff
87a3e92+dirty 1231.00 ms 1229.25 ms -1.75 ms

App size

Revision Plain With Sentry Diff
87a3e92+dirty 2.92 MiB 3.63 MiB 732.48 KiB

Copy link
Contributor

github-actions bot commented Aug 28, 2024

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 384.80 ms 417.79 ms 33.00 ms
Size 7.15 MiB 8.33 MiB 1.18 MiB

Previous results on branch: kw/ref-native-frames-prepare-for-span-measurements

Startup times

Revision Plain With Sentry Diff
87a3e92+dirty 362.11 ms 435.40 ms 73.28 ms

App size

Revision Plain With Sentry Diff
87a3e92+dirty 7.15 MiB 8.33 MiB 1.18 MiB

Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

Non-blocking questions and also some minor suggestions. Let me know if you need more reviews.

@krystofwoldrich krystofwoldrich merged commit 5aa6aff into v6 Sep 2, 2024
57 of 58 checks passed
@krystofwoldrich krystofwoldrich deleted the kw/ref-native-frames-prepare-for-span-measurements branch September 2, 2024 14:45
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.

2 participants