Handle shim data from GLEAN #74
Conversation
| @@ -24,6 +24,10 @@ def handle_message(event, context): | |||
| for tile in telemetry["tiles"]: | |||
| if "shim" in tile: | |||
| ping_adzerk(tile['shim']) | |||
| elif "metrics" in telemetry: | |||
There was a problem hiding this comment.
The ping, as documented, is sent for both clicks and impressions. We should check ping_info.reason to see what this ping is about and bubble the information accordingly as needed.
There was a problem hiding this comment.
I don't know what is meant here by "bubble the information accordingly".
I suspect you mean for it to be added to the value passed to ping_adzerk. That function is being passed a checksummed string, so it's unclear to me how that would be done, or even what the string contains, and I assumed that pocket was formatting the value in the client with all necessary information.
There was a problem hiding this comment.
I mean that this should be something like
| elif "metrics" in telemetry: | |
| elif ("ping_info" in telemetry") and ("metrics" in telemetry): | |
| # reason is either a 'click' or an 'impression'. | |
| reason = ping_info.reason |
If adzerk is only expecting 'impressons', then we should not submit the SHIM in case this is a 'click' (user clicked on the ad).
There was a problem hiding this comment.
@Dexterp37, Kevel (formerly AdZerk) are expecting shims for both clicks and impressions.
There was a problem hiding this comment.
@nina-py does it mean that AdZerk doesn't care about the difference between clicks and impression and so we should not send additional information, or that we should send this particular bit of information in addition to the SHIM?
There was a problem hiding this comment.
@Dexterp37, from what I know, the shim contains all the info Kevel needs, including whether it's an impression or a click, so we should be good there sending just the shim as we do from the desktop browser.
|
@relud, could you please do an empty commit to trigger a CI check? As initially it wouldn't start because your patch is coming from a fork of this repository. We've updated settings now but need a commit to kickstart the CI check. |
|
looks like still no response from CI |
|
@relud, I wonder if you have permissions to create a branch on this repo directly and recreate this PR so that CircleCI would do the checks? Would this be too much to ask? 🙏 |
|
I don't have permission to do that, but just pushing my commit to a branch on this repo instead of the fork might do the trick (it does for other repos owned by DE). I also don't have permissions to do that, but these are the commands that would do it: git clone https://github.com/Pocket/proxy-server
cd proxy-server
git remote add relud https://github.com/relud/proxy-server
git fetch relud
# this is the part that i don't have permission to do
git push origin refs/remotes/relud/patch-1:refs/heads/relud-patch-1 |
|
@relud, I just did that following your instructions, looks like you can create a new PR now? https://github.com/Pocket/proxy-server/compare/relud-patch-1?expand=1 |
|
Meanwhile, CircleCI continues being weird - it did run a check on this branch but failed installing things 😵💫 |
|
new PR created: #75 |
|
Ah thanks @relud ! and apologies - made the new PRs to show alternatives with your changes then was going to rebase to not make your experience merging this one so bumpy, but alas, i made things bumpy haha |
handle data sent by mozilla-mobile/fenix#27550 for FNXV2-21791 as prescribed in google doc from klukas
blocks https://github.com/mozilla-services/cloudops-infra/pull/4250