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(dynamic-sampling): Split function for sampling match #2047

Merged
merged 4 commits into from
Apr 25, 2023

Conversation

iambriccardo
Copy link
Member

@iambriccardo iambriccardo commented Apr 19, 2023

This PR is a preparation for the refactoring of dynamic sampling. It contains the splitting of the matching function in order to decouple ProjectState and SamplingConfig.

@iambriccardo iambriccardo marked this pull request as ready for review April 19, 2023 07:30
@iambriccardo iambriccardo requested a review from a team April 19, 2023 07:30
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to improve the code quality, and splitting up the original PR!

Please see the comment on the pub before merging, and consider continuing the refactor in follow-up PRs.

@@ -50,21 +50,15 @@ fn check_unsupported_rules(

/// Gets the sampling match result by creating the merged configuration and matching it against
/// the sampling configuration.
fn get_sampling_match_result(
pub fn merge_configs_and_match(
Copy link
Contributor

Choose a reason for hiding this comment

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

If you think about the semantical usage of this function, there're two steps that we're doing here. It'd be great if we could split this function into the merging and matching functions. Could we do this in a follow-up PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehm, they are already split up in lines 66 and 67. This function orchestrates their behavior, this is why it is named like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking something like the following:

fn get_sampling_match() {
  merge_configs();
  match_configs();
}

Copy link
Member Author

@iambriccardo iambriccardo Apr 25, 2023

Choose a reason for hiding this comment

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

I don't think that the complexity of the code requires the creation of a third function encapsulating that behavior. If it was more complex I would have agreed. On the other hand, if the encapsulation follows some strict Relay guidelines I am happy to adapt.

relay-server/src/utils/dynamic_sampling.rs Outdated Show resolved Hide resolved
@jjbayer jjbayer merged commit daa9dcb into master Apr 25, 2023
@jjbayer jjbayer deleted the riccardo/ref/dynamic-sampling-split branch April 25, 2023 11:37
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