-
Notifications
You must be signed in to change notification settings - Fork 8
Add bidder params and debug helpers to openrtb request #172
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
base: fix/proxy-compression-issues
Are you sure you want to change the base?
Add bidder params and debug helpers to openrtb request #172
Conversation
| })); | ||
| } | ||
|
|
||
| // Parse bids from query parameter if present |
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 know this is draft but can we finally move to specific structs.
fbae661 to
1f11d06
Compare
1543ff3 to
dac30b3
Compare
dac30b3 to
d199f21
Compare
9de521a to
d0b99c1
Compare
aram356
left a 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.
There is some opportunity to improve it.
| let mut page_url = format!("https://{}", settings.publisher.domain); | ||
|
|
||
| // Append debug query params if configured | ||
| if let Some(ref params) = config.debug_query_params { |
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.
⛏️ similar on 827 and 280
extract to a function
fn append_query_params(url: &str, params: &str) -> String {
if params.is_empty() || url.contains(params) {
return url.to_string();
}
if url.contains('?') {
format!("{}&{}", url, params)
} else {
format!("{}?{}", url, params)
}
}
| } | ||
|
|
||
| // Add debug flag if enabled | ||
| if self.config.debug { |
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 use concrete types
| } | ||
| } | ||
|
|
||
| log::info!("Prebid OpenRTB request: {:#?}", openrtb_json); |
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.
❓ Should we be logging the whole request? At least change to debug
| } | ||
|
|
||
| let body_bytes = response.take_body_bytes(); | ||
| log::info!("Prebid OpenRTB response: {}", String::from_utf8_lossy(&body_bytes)); |
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.
❓ Should we log the whole response? At least change to debug
| bidder.insert(bidder_name.clone(), Json::Object(serde_json::Map::new())); | ||
| } | ||
| // Use bidder params from the slot (passed through from the request) | ||
| let bidder: std::collections::HashMap<String, Json> = slot |
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.
❓ What happens if there no bidders. Should we the settings?
Waiting until #147 is merged.