Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle shim data from GLEAN #74

Closed
wants to merge 2 commits into from
+4 −0
Closed

Handle shim data from GLEAN #74

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6634f68
Handle shim data from GLEAN
relud Nov 10, 2022
7c46f35
trigger ci
relud Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/telemetry/handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that this should be something like

Suggested change
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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Dexterp37, Kevel (formerly AdZerk) are expecting shims for both clicks and impressions.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

text_metrics = telemetry["metrics"].get("text", {})
if "pocket.spoc_shim" in text_metrics:
ping_adzerk(text_metrics["pocket.spoc_shim"])


def ping_adzerk(shim):
Expand Down