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 mixed profiles (profile event V2) #2685

Closed
wants to merge 2 commits into from

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Nov 2, 2023

This PR drafts a structure of mixed profiles which enable us to ingest for example Mixed React Native and Android profiles.

Copy link
Contributor

@phacops phacops left a comment

Choose a reason for hiding this comment

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

I don't think creating a mixed format like this is the right approach. We could add a field to the current Android format to make it backwards compatible (we wouldn't need any change to the pipeline if the field is empty).

You can still reuse some elements of the sample format (like the Profile struct) but besides that, let's keep things separate.

@krystofwoldrich
Copy link
Member Author

krystofwoldrich commented Nov 2, 2023

we wouldn't need any change to the pipeline if the field is empty

I agree adding an extra field on the Android profile is a faster way to go, but it would work only for Android.

The mixed format would allow a general approach for Hybrid SDKs to pack multiple profiles. Compared to that adding a field to the Android profile works only on Android. (Mixed profiles could be used for iOS, Hermes...)

Adding a field on the Android profile moves part of the complexity to SDKs, where the assembled event will change metadata based on the content. (Hermes -> Profiles V1, Hermes + iOS -> Profiles V1, Hermes + Android -> Android Profiles "V0")

The SDK should send one format of metadata no matter the actual profiling data. If needed to reduce the complexity we can change the format in Relay.

I'll adjust the PR, the mentioned points can be addressed in the future.

@phacops
Copy link
Contributor

phacops commented Nov 2, 2023

Do we actually have to create another format for sample profilers? Why wouldn't we add the samples/stacks/frames the way we have the schema already, mixed in with the rest with adding the proper platform field to frames? I'm not convinced we need a new mixed schema.

Android has always been treated differently because of the binary format it sends us not fitting anything else. For the rest, it seems like the sample format we created fits our needs, even with RN frames and samples mixed in with native frames and samples.

@krystofwoldrich
Copy link
Member Author

krystofwoldrich commented Nov 3, 2023

The current format works for mixed profiles, but we mixed them on a device at the moment. We also convert RN Hermes profiles on a device. It's still a time-consuming task, but faster than parsing the Android binary trace.

I'm not sure about adding rnProfile property to the Android profile is:

  • The RN SDK controls the Transactions and Profiles.
  • The top-level platform still should be javascript not android.
  • The Android Metadata is different from the V1 profiles. (The RN SDK could create the Android format of metadata, but it seems more like a workaround)

But as we would like to have our own Android profiler in the future which will produce our format, which will be simpler to process and merge on a device, maybe the workaround is acceptable in this case.

@krystofwoldrich
Copy link
Member Author

Closing this in favor of simpler RN/Android specific format -> #2706

@jan-auer jan-auer deleted the kw-mixed-profiles-android branch July 4, 2024 11:47
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.

2 participants