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

ref(profiling): emit (processed) profile outcomes in profiling consumers #80189

Merged

Conversation

viglia
Copy link
Contributor

@viglia viglia commented Nov 4, 2024

@viglia viglia self-assigned this Nov 4, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 4, 2024
Copy link

codecov bot commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 36.66667% with 19 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/sentry/profiles/task.py 28.00% 18 Missing ⚠️
src/sentry/ingest/billing_metrics_consumer.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #80189      +/-   ##
==========================================
- Coverage   78.09%   78.08%   -0.01%     
==========================================
  Files        7178     7176       -2     
  Lines      317128   317142      +14     
  Branches    43719    43722       +3     
==========================================
+ Hits       247649   247651       +2     
+ Misses      63136    63135       -1     
- Partials     6343     6356      +13     

src/sentry/profiles/task.py Outdated Show resolved Hide resolved
src/sentry/profiles/task.py Outdated Show resolved Hide resolved
tests/sentry/profiles/test_task.py Show resolved Hide resolved
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

process_profile_task has a lot of code paths that exit/return without emitting an outcome, all of these cases now would never emit an outcome, I don't think this is desired, I assume that these at least should be counted in some error category.

@viglia
Copy link
Contributor Author

viglia commented Nov 5, 2024

process_profile_task has a lot of code paths that exit/return without emitting an outcome, all of these cases now would never emit an outcome, I don't think this is desired, I assume that these at least should be counted in some error category.

@Dav1dde as we discussed quickly over slack, all the early return are taken care of (inside the functions body when we catch the exception).

The only place where we return early and not emit an outcome is here at the very beginning where we assert profile is not None.
If that fails, i.e. profile is None, then I couldn't even distinguish between a profile and a chunk, so I'm not sure which outcome should be emitted in that case.

@viglia viglia requested a review from jjbayer November 5, 2024 17:25
@viglia viglia merged commit 8484357 into master Nov 7, 2024
49 of 50 checks passed
@viglia viglia deleted the viglia/ref/emit-profile-outcomes-in-profiling-consumers branch November 7, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants