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): remove profiling support #595

Merged
merged 1 commit into from Jun 27, 2023

Conversation

viglia
Copy link
Contributor

@viglia viglia commented Jun 27, 2023

The profiler produced questionable sample rates, we frequently saw gaps that were multiple seconds long. This meant that we couldnt not have strong confidence in the quality of the data

@viglia viglia requested a review from Swatinem June 27, 2023 11:27
@viglia viglia self-assigned this Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #595 (867f9af) into master (3ab112c) will increase coverage by 2.50%.
The diff coverage is 100.00%.

❗ Current head 867f9af differs from pull request most recent head 3e6e0da. Consider uploading reports for the commit 3e6e0da to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   70.80%   73.30%   +2.50%     
==========================================
  Files          67       65       -2     
  Lines        6857     6579     -278     
==========================================
- Hits         4855     4823      -32     
+ Misses       2002     1756     -246     

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

sad to see it go. we were quite close turning this on in S4S :-D

@viglia viglia merged commit 3e687f1 into master Jun 27, 2023
12 checks passed
@viglia viglia deleted the viglia/feat/remove-profiling-feature branch June 27, 2023 14:18
FGRibreau pushed a commit to hook0/hook0 that referenced this pull request Aug 24, 2023
As they are going to drop it in the next release: getsentry/sentry-rust#595
@penso
Copy link

penso commented Oct 16, 2023

Would love to see some profiling back to Sentry/Rust.

@outductor
Copy link

Any chance profiling comes back in the future roadmap?

@Swatinem
Copy link
Member

The problem we had with this implementation was that the sampling accuracy was very low, which means we did not have samples takes at predictable intervals, of predictable threads.

Coupled with the fact that this whole product feature was tied to the performance monitoring product, which has rather short-lived transactions of a single execution flow.
That, together with the fact that we have multithreaded work-stealing executors for async Rust means that this was a bad fit.
The performance-monitoring part, which is powered primarily by #[tracing::instrument] and thus very accurate, does not really work well with sampling profiling if you consider that the code you are instrumenting is most likely async which can yield to other threads and also change threads.
It is thus very hard to relate the samples collected in profiling to the actual transaction / span you want to profile.

Long story short, we decided to rather remove this integration from the SDK, rather than have a suboptimal experience.

I totally understand the interest in this feature though, and I will make sure to forward this feedback internally.

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.

None yet

4 participants