feat(routing): Integrate global success rates #6950
+172
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with Changed Files
|
Chethan-rao
requested changes
Dec 30, 2024
| @@ -809,6 +811,13 @@ pub struct CurrentBlockThreshold { | |||
| pub max_total_count: Option<u64>, | |||
| } | |||
|
|
|||
| #[derive(serde::Serialize, serde::Deserialize, Debug, Default, Clone, ToSchema)] | |||
| pub enum SuccessRateSpecificityLevel { | |||
There was a problem hiding this comment.
this type has to be mentioned in openapi.rs files?
Comment on lines
+861
to
+863
| if let Some(level) = new.specificity_level { | ||
| self.specificity_level = Some(level) | ||
| } |
There was a problem hiding this comment.
why?
Suggested change
| if let Some(level) = new.specificity_level { | |
| self.specificity_level = Some(level) | |
| } | |
| self.specificity_level = new.specificity_level; |
| @@ -49,6 +52,15 @@ pub trait SuccessBasedDynamicRouting: dyn_clone::DynClone + Send + Sync { | |||
| id: String, | |||
| headers: GrpcHeaders, | |||
| ) -> DynamicRoutingResult<InvalidateWindowsResponse>; | |||
| /// To calculate both global and merchant specific success rate for the list of chosen connectors | |||
| async fn calculate_global_success_rate( | |||
There was a problem hiding this comment.
include merchant too here in method name
| .fetch_entity_and_global_success_rate(request) | ||
| .await | ||
| .change_context(DynamicRoutingError::SuccessRateBasedRoutingFailure( | ||
| "Failed to fetch the success rate".to_string(), |
There was a problem hiding this comment.
Suggested change
| "Failed to fetch the success rate".to_string(), | |
| "Failed to fetch the entity and global success rate".to_string(), |
| .first() | ||
| .ok_or(errors::ApiErrorResponse::InternalServerError) | ||
| .attach_printable( | ||
| "unable to fetch the first connector from list of connectors obtained from dynamic routing service", |
There was a problem hiding this comment.
Suggested change
| "unable to fetch the first connector from list of connectors obtained from dynamic routing service", | |
| "unable to fetch the first connector from list of global connectors obtained from dynamic routing service", |
| ))?; | ||
|
|
||
| let outcome = get_success_based_metrics_outcome_for_payment( | ||
| payment_status_attribute, | ||
| payment_connector.to_string(), | ||
| first_success_based_connector.to_string(), | ||
| first_merchant_success_based_connector.to_string(), | ||
| ); | ||
|
|
||
| let dynamic_routing_stats = DynamicRoutingStatsNew { |
There was a problem hiding this comment.
should we create a new column here which stores the globally_top_performing_connector at the time this particular payment was made?
cc: @prajjwalkumar17
|
there's conflicts too. please add test cases as well |
Chethan-rao
reviewed
Dec 30, 2024
Comment on lines
+814
to
+815
| #[derive(serde::Serialize, serde::Deserialize, Debug, Default, Clone, ToSchema)] | ||
| pub enum SuccessRateSpecificityLevel { |
There was a problem hiding this comment.
Suggested change
| #[derive(serde::Serialize, serde::Deserialize, Debug, Default, Clone, ToSchema)] | |
| pub enum SuccessRateSpecificityLevel { | |
| #[derive(serde::Serialize, serde::Deserialize, Debug, Default, Clone, ToSchema)] | |
| #[serde(rename_all = "snake_case")] | |
| pub enum SuccessRateSpecificityLevel { |
Chethan-rao
requested changes
Dec 30, 2024
| @@ -801,6 +802,7 @@ pub struct SuccessBasedRoutingConfigBody { | |||
| pub default_success_rate: Option<f64>, | |||
| pub max_aggregates_size: Option<u32>, | |||
| pub current_block_threshold: Option<CurrentBlockThreshold>, | |||
| pub specificity_level: Option<SuccessRateSpecificityLevel>, | |||
There was a problem hiding this comment.
Instead of making this optional and manually filling this field for the old entries, we can make this a required field but derive a #[serde(default)] attribute on top so that when getting deserialised, serde will take care of setting this to a default value instead of you doing it. this shouldn't cause any backwards compatibility issues. what do you think?
Suggested change
| pub specificity_level: Option<SuccessRateSpecificityLevel>, | |
| #[serde(default)] | |
| pub specificity_level: SuccessRateSpecificityLevel, |
Comment on lines
+1343
to
+1346
| success_based_routing_configs.config.as_mut().map(|config| { | ||
| config.specificity_level = | ||
| Some(api_models::routing::SuccessRateSpecificityLevel::Merchant) | ||
| }); |
There was a problem hiding this comment.
you can remove this now because serde will take care of it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of Change
Description
Integrate global Success rates for dynamic routing
Additional Changes
Motivation and Context
How did you test it?
Checklist
cargo +nightly fmt --allcargo clippy