-
Notifications
You must be signed in to change notification settings - Fork 94
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 optional js profile to Android profiles events #2706
Conversation
The PR is ready for review. Haven't tested it locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. Overall looks ok. I left few comments which I think should be addressed before we continue.
It would be also a good idea to add more unit tests to see if the things you expect end up in the correct places.
This code is not new, I just moved it to the Profile. There are tests for the functions in relay/relay-profiling/src/sample.rs Line 411 in a0e342d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am commenting on a lot of code that was moved (only saw that after), but maybe now is a good time to clean this up a bit.
@@ -221,6 +238,17 @@ mod tests { | |||
); | |||
} | |||
|
|||
#[test] | |||
fn test_roundtrip_react_native() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test validates the parsing doesn't fail, but we don't know if the contents are what we expect. What do you think about validating that too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests ensuring values are in current places are in sample.rs
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sample profile fixture seems a bit too excessive with 969KB, is this the minimal profile we can use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's not the minimum, but it's an improvement compared to the previous roundtrip test (1,5 MB).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for cleaning this up!
Thanks for the ✅ I'm waiting for one from the profiling team and then this can be merged. |
This PR drafts a structure of mixed profiles which enable us to ingest for example Mixed React Native and Android profiles.