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.

Sign up for GitHub

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

Jump to bottom

revert(profiling): Remove concurrent profiling #1697

Merged
merged 22 commits into from
Dec 15, 2022
Merged

revert(profiling): Remove concurrent profiling #1697

merged 22 commits into from
Dec 15, 2022

Conversation

phacops
Copy link
Contributor

@phacops phacops commented Dec 15, 2022 •
edited
Loading

This piece of code was duplicating profiles and trimming them to the right duration for each transaction they were associated with.

As we want to be able to only accept profiles associated with a transaction, we have to send the profile in the same envelope as transaction so dynamic sampling works properly. We can't start multiple profilers at the time (Android API we're using doesn't allow that) or we can't do the processing the server was doing to get one profile and trim on the client. We will keep trimming sample and cocoa formats.

Therefore, we decided to remove this concurrent profiling support.

@phacops phacops requested a review from a team as a code owner December 15, 2022 02:34
@phacops phacops requested a review from a team December 15, 2022 02:34
jan-auer
jan-auer reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

This changes android profile parsing, yet there's still unlimited expansion in expand_cocoa_profile and expand_sample_profile. Shouldn't we remove that, too?

@jan-auer
Copy link
Member

test_expand_multiple_transactions is still failing now. It asserts that events of the profile have been filtered, but expansion no longer does that. @phacops please confirm if that can be changed in the test.

@phacops phacops changed the title revert(profiling): Remove concurrent profiling for Android revert(profiling): Remove concurrent profiling Dec 15, 2022
@phacops phacops enabled auto-merge (squash) December 15, 2022 17:29
@phacops phacops disabled auto-merge December 15, 2022 18:26
jan-auer
jan-auer reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Preliminary review of Android.

jan-auer
jan-auer reviewed Dec 15, 2022
View reviewed changes
@jan-auer
Merge branch 'master' into pierre/profiling-remove-concurrent-profili…
98ba5e2 
…ng-for-android

* master:
  chore(toolchain): Bump Rust toolchain to 1.66.0 (#1699)
  release: 22.12.0
  fix: Properly escape generic type in docstring (#1701)
@jan-auer
meta: Changelog
0cba102
@jan-auer jan-auer enabled auto-merge (squash) December 15, 2022 22:00
@jan-auer jan-auer merged commit d375639 into master Dec 15, 2022
@jan-auer jan-auer deleted the pierre/profiling-remove-concurrent-profiling-for-android branch December 15, 2022 22:15
@jan-auer jan-auer added this to the Incident milestone Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jan-auer jan-auer jan-auer approved these changes

@Zylphrex Zylphrex Zylphrex approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Incident
Development

Successfully merging this pull request may close these issues.
3 participants
@phacops @jan-auer @Zylphrex