feat(ads-client): roundtrip cap key for spoc impression callbacks (AC-125)#7442
feat(ads-client): roundtrip cap key for spoc impression callbacks (AC-125)#7442jonesetc wants to merge 2 commits into
Conversation
5ce339a to
bc47c9b
Compare
|
|
||
| fn impression_enriching_params(&self) -> Vec<(String, String)> { | ||
| vec![("cap_key".to_owned(), self.caps.cap_key.clone())] | ||
| } |
There was a problem hiding this comment.
I think you can do fn impression_enriching_params(&self) -> [(&str, &str); 1] to have zero allocation (no Vec, no String)
There was a problem hiding this comment.
The problem with the &str (or any reference returned) in there is that this result is used during the mutable borrow of callbacks_mut, so those would have to be cloned anyway. I think returning something like Option<IntoIterator<(String, String)>> should be possible and avoid allocation in the non-spoc case and just a single small array in the spoc case.
There was a problem hiding this comment.
I just went ahead and simplified it instead of worrying about iterators or refs. Now there is just a method that returns an optional cap_key, by default None, and spoc returns a string. The string would need to be owned eventually like i mentioned because of the mutable borrow for callbacks. so I figured just returning the string made more sense than making the caller do something like:
let cap_key = ad.cap_key().map(String::from)| .query_pairs_mut() | ||
| .append_pair("request_hash", &hash_str); | ||
| .append_pair("request_hash", &hash_str) | ||
| .extend_pairs(impression_enriching_params); |
There was a problem hiding this comment.
Since we already have the request_hash acting as a unique identifier up top, could storing it in the SQLite table "impression_log" be a cleaner alternative here?
There was a problem hiding this comment.
A request hash maps to an an ad response, each of which contains 0-* spocs. Each spoc has its own cap key. So going just from the request hash to a single cap key doesn't work.
A much messier thing that could be done would be to just map the impression url itself to the block key, but that is assuming that the mapping between spoc:impression url:cap key is always 1:1:1. Actually getting back to the cap key directly makes the fewest assumptions and allows the cap keys to do more complex things from the server like "all of these spocs have cap key A, and these have cap key B" to have group limits instead of just per individual spoc.
bc47c9b to
e50e0d5
Compare
Almaju
left a comment
There was a problem hiding this comment.
Thanks for the changes, lgtm!
We might be able refactor this later with async model because we will need some AdStore that can hold stateful data such as request hash or cap key but this looks great for now and the current sync stateless model we have!
Add roundtripping of
cap_keyin sposored content impression callback urls. This value is currently just discarded, but in the future will be used to keep a rolling count of impressions in the last 24 hours on device to make sure sponsored content is not being shown too often.This does not change any internal or external APIs, and does not require an entry in the changelog
Pull Request checklist
[ci full]to the PR title.