-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Webhook trigger for experiments #265
base: main
Are you sure you want to change the base?
Conversation
9e4651e
to
0e93efa
Compare
9b958f5
to
de69732
Compare
crates/experimentation_platform/src/api/experiments/handlers.rs
Outdated
Show resolved
Hide resolved
crates/experimentation_platform/src/api/experiments/handlers.rs
Outdated
Show resolved
Hide resolved
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.
few sections' review pending
@@ -119,6 +122,11 @@ impl error::ResponseError for AppError { | |||
StatusCode::INTERNAL_SERVER_ERROR, | |||
"Something went wrong", | |||
), | |||
|
|||
AppError::WebhookError(error) => Self::generate_err_response( | |||
StatusCode::from_u16(512).unwrap_or(StatusCode::INTERNAL_SERVER_ERROR), |
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.
suggestion:
should we make this StatusCode::from_u16(512)
a const using lazy_static ?
tenant_config.experiments_webhook_config, | ||
inserted_experiment, | ||
&config_version_id, | ||
WebhookEvent::ExperimentCreated, |
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.
instead of passing these values from the different places from where it is called, shouldn't we just pass the route to this function, and the value of the WebhookEvent
here could be auto inferred via an implementation of
WebhookEvent: From<String>
this it keeps the mapping between the routes and EventName
in one single, also helping in avoiding situations wherein the dev might send the wrong value from the place of call, for that particular route
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 guess this is fine, it would be an overkill i guess.
if the dev sends wrong value, it will be reviewed.
crates/experimentation_platform/src/api/experiments/handlers.rs
Outdated
Show resolved
Hide resolved
de69732
to
2ad138a
Compare
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.
pending comments plus these as all
crates/service_utils/src/helpers.rs
Outdated
let mut header_array = webhook | ||
.headers | ||
.into_iter() | ||
.filter_map(|key| match key { |
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.
why do we need to do a filter operation over here ?
this is counter intuitive, webhook.headers
, is of type Vec<HeadersEnum>
, so it does not make sense to filter here
to avoid repetition of keys in the headers (which headers anyways disregards and keeps only one entry as its a map), you can simply add if checks for each value present in the HeadersEnum
which makes the handling of each header explicit, as it is supposed to be different behaviour for each
or this could have simply been a map
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.
But config_version_id is optional, so filter_map
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.
yeah, that why is better to individually do if checks for all the enums
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.
How please explain
crates/service_utils/src/helpers.rs
Outdated
Ok(()) | ||
} | ||
_ => { | ||
log::error!("Webhook call failed with status code: {:?}, response headers: {:?}", res.status(), res.headers()); |
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.
shouldn't we log the response body here, instead of the response headers ?
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.
cc: @Datron
4752a25
to
e2b98fc
Compare
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.
Looks good, few questions
tenant_config.experiments_webhook_config.enabled, | ||
tenant_config.experiments_webhook_config.configuration, |
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 thought we added enabled to avoid parsing configuration?
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.
But this check would be needed right?
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.
mostly pending comments
D: Deserializer<'de>, | ||
{ | ||
#[derive(Deserialize)] | ||
struct WebhookConfigHelper { |
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 dont need these, right ?
the type here is still Option<Webhook>
, so this does not help much
wherever this data is to be consumed, you can directly check
if webhook.enabled { webhook.configuration.unwrap() }
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.
Isn't it better to throw proper error than unwraping
(true, None) => Err(serde::de::Error::custom( | ||
"Configuration must be provided when enabled is true", | ||
)), | ||
(false, Some(_)) => Err(serde::de::Error::custom( |
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.
you don't need to have this case, because the .toml
schema gives the guarantee that it is supposed to be correct
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.
How will the .toml figure this pattern out?
crates/service_utils/src/helpers.rs
Outdated
let mut header_array = webhook | ||
.headers | ||
.into_iter() | ||
.filter_map(|key| match key { |
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.
yeah, that why is better to individually do if checks for all the enums
.filter_map(|key| match key { | ||
HeadersEnum::ConfigVersion => { | ||
if let Some(config_version) = config_version_opt { | ||
Some(("x-config-version".to_string(), config_version.to_string())) |
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.
this is pending: #265 (comment)
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.
please explain with code, i didn't get you here.
} | ||
|
||
#[derive(Serialize, Deserialize)] | ||
pub enum WebhookEvent { |
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.
if the key is going to be experiments_webhook_config
then the Enum name should also be renamed to ExperimentWebhookEvent
, as these are not re-usable for cac
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.
Why is this not usable for CAC, the webhook object will be similar, it can be reused. If it can't be reused then it can be renamed then. But now i think this is fine
let response = ExperimentCreateResponse::from(inserted_experiment); | ||
|
||
let response = ExperimentCreateResponse::from(inserted_experiment.clone()); | ||
if let (true, Some(experiments_webhook_config)) = ( |
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.
wont it it be better to abstract it out inside execute_webhook_call
itself
as this is being repeated in all the places
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.
cc: @Datron
@@ -648,6 +660,29 @@ async fn ramp( | |||
)) | |||
.get_result(&mut conn)?; | |||
|
|||
let (_, config_version_id) = fetch_cac_config(&tenant, &data).await?; | |||
|
|||
let webhook_event = if new_traffic_percentage == 0 |
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.
if the new ramp is 0 and previous ramp is not 0, but something else like 10 or 20, in that case it should not be ExperimentStarted
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.
But if the previous ramp is not 0, then the previous experimentStatus won't be CREATED, it would be INPROGRESS
80e79d4
to
5754a6d
Compare
5754a6d
to
0dbd476
Compare
Problem
Webhook trigger for experiments
Solution
Webhook trigger for experiments (Create, Conclude, ramp, update_overrides)
Tenant Config changes
experiments_webhook_config = { "value" = {}, "schema" = { "type" = "object", properties = {"url" = { "type" = "string" }, "method" = { "enum" = ["Post","Put","Get"], "type" = "string" },"headers" = { "type" = "array", "items" = { "type" = "string", "enum" = ["ConfigVersion"] }}, authorization = {"type" = "object", properties = { "key" = { "type" = "string" }, "value" = { "type" = "string" }}}, "required"= ["url", "method", "headers", "authorization"], "additionalProperties" = false}}}
Post-deployment activity
Tenant Config Changes