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

feat(profiling): Add Native iOS profiles to Hermes JS profiles #3349

Merged
merged 48 commits into from
Nov 17, 2023

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Oct 17, 2023

📢 Type of change

  • New feature

📜 Description

This PR adds native iOS profiles to currently existing RN Hermes profiles.

Hermes and Native profiles are merged on a device and sent as one profiling event in the envelope with associated Transaction.

💡 Motivation and Context

💚 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

🔮 Next steps

@krystofwoldrich krystofwoldrich changed the title Add new profile event handling and basic merge feat(profiling): Add Native iOS profiles to Hermes JS profiles Oct 17, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 17, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 665f1d7

@krystofwoldrich krystofwoldrich changed the base branch from main to kw-symbolicated-profiles October 19, 2023 13:50
@krystofwoldrich krystofwoldrich marked this pull request as ready for review October 30, 2023 12:48
@krystofwoldrich krystofwoldrich mentioned this pull request Nov 7, 2023
Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Couple quick things from me!

ios/RNSentry.mm Outdated Show resolved Hide resolved
ios/RNSentry.mm Outdated Show resolved Hide resolved
ios/RNSentry.mm Show resolved Hide resolved
@krystofwoldrich
Copy link
Member Author

@armcknight @lucas-zimerman Thank you for the feedback. I've adjusted to code, can you take one more look?

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Native code changes in this PR look good, nice work.

I noticed some preexisting code not covered in the diff, so I couldn't comment directly on it, but is this macro left undefined in the main branch, and you just define it to 1 if you're debugging locally?

#if SENTRY_PROFILING_DEBUG_ENABLED

@krystofwoldrich
Copy link
Member Author

@armcknight Yes, exactly.

@krystofwoldrich krystofwoldrich merged commit e9fea85 into main Nov 17, 2023
50 of 51 checks passed
@krystofwoldrich krystofwoldrich deleted the kw-mixed-profiling branch November 17, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants