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): forward profiles of non-sampled transactions (with no options filtering) #3963

Merged

Conversation

viglia
Copy link
Contributor

@viglia viglia commented Aug 28, 2024

Remove the options to shutdown, filter by platform or downsample the number of processed profiles we pass along to extract the metrics.

With this, all the Processed Profiles (that are not indexed), will be passed along, as we currently do, but without the further checks that are currently in place (rollout rate, platforms, etc.).

The global kill-switch is left, should we ever need it in case of incidents.

remove the options to shutdown, filter by platform or downsample the
number of processed profiles we pass along to extract the metrics.
@viglia viglia self-assigned this Aug 28, 2024
@viglia viglia marked this pull request as ready for review August 28, 2024 15:55
@viglia viglia requested a review from a team as a code owner August 28, 2024 15:55
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

I like this change because it reduces complexity. But are we confident that we can sustain ingesting all profiles for the foreseeable future? Would it make sense to keep at least the global kill switch so we have some control during incidents?

relay-server/src/services/processor.rs Outdated Show resolved Hide resolved
@viglia
Copy link
Contributor Author

viglia commented Aug 29, 2024

I like this change because it reduces complexity. But are we confident that we can sustain ingesting all profiles for the foreseeable future? Would it make sense to keep at least the global kill switch so we have some control during incidents?

@jjbayer yes, generally speaking we're sure we want to forward all the processed profiles for the foreseeable future. Still, I agree that we should still keep the global kill switch, in case it's ever needed. I'll put that one back 👍🏻

add back `profiling.profile_metrics.unsampled_profiles.enabled` global
kill switch.
@viglia viglia requested review from Dav1dde and jjbayer August 29, 2024 08:50
@viglia viglia changed the title ref(profiling): ingest all unsampled profiles ref(profiling): forward profiles of non-sampled transactions (with no options filtering) Aug 29, 2024
relay-dynamic-config/src/feature.rs Show resolved Hide resolved
relay-ua/uap-core Outdated Show resolved Hide resolved
tests/integration/test_outcome.py Outdated Show resolved Hide resolved
viglia added 2 commits August 29, 2024 13:02
add deprecated feature for projects:profiling-ingest-unsampled-profiles
define the following as deprecated:
* `profiling.profile_metrics.unsampled_profiles.platforms`
* `profiling.profile_metrics.unsampled_profiles.sample_rate`
@viglia viglia requested a review from jjbayer August 29, 2024 12:46
Copy link
Member

@jjbayer jjbayer left a comment

Choose a reason for hiding this comment

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

Please fix the uap-core diff as discussed, after that this looks good to go!

@viglia viglia requested a review from a team August 29, 2024 15:29
@viglia viglia merged commit b2ab8c2 into master Aug 30, 2024
25 checks passed
@viglia viglia deleted the viglia/ref/remove-options-and-ingest-all-unsampled-profiles branch August 30, 2024 09:23
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.

3 participants