-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix(profiling): Only transfer valid profile ids #3809
Conversation
This was transfering the profile id onto the transaction all the time which is not desirable since we need to fully validate the profile first.
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.
We transfer the profile ID into the event before dynamic sampling to enable a specific dynamic sampling bias, see #2710.
If this dynamic sampling behavior is no longer needed, we can go ahead with the PR and integrate the transfer into profile::process
as David suggested.
@jjbayer Ahh I wasn't aware of that. The issue I'm trying to address here is that by transferring the profile id before processing the profile, it's possible that the profile is invalid and references should be removed from the transaction as well. I've updated the PR to transfer the profile id twice. Once for dynamic sampling, and once after the processing to handle this. Are there any issues with this? |
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've updated the PR to transfer the profile id twice. Once for dynamic sampling, and once after the processing to handle this. Are there any issues with this?
That should work, yes.
This was transfering the profile id onto the transaction all the time which is not desirable since we need to fully validate the profile first.