Skip to content

[AE-782] Build dap collector job for incrementality experiments #387

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
Open
wants to merge 43 commits into
base: main
Choose a base branch
from
Open

[AE-782] Build dap collector job for incrementality experiments #387

wants to merge 43 commits into from
+1,938 −1

Conversation

mashalifshin
Copy link

@mashalifshin mashalifshin commented Aug 18, 2025 •
edited
Loading

Problem Statement:

Currently, we are running incrementality experiments on FFHNT, measuring whether our Tiles are effective by seeing if they give a lift to organic visits, or to checkouts, to the tile advertiser. We're doing this in a private, anonymized manner, by sending the experiment results to DAP. However, we have no way to collect those results from DAP in an automated way and bring them into our usual data pipeline, so those results are currently being manually collected by an engineer.

Solution:

Create a docker-etl job that can go out to Nimbus and find out how to collect the results for each experiment branch, go out and collect those results from DAP, and finally write those results into BQ.

Checklist for reviewer:

  • Commits should reference a bug or github issue, if relevant (if a bug is
    referenced, the pull request should include the bug number in the title)
  • Scan the PR and verify that no changes (particularly to
    .circleci/config.yml) will cause environment variables (particularly
    credentials) to be exposed in test logs
  • Ensure the container image will be using permissions granted to
    telemetry-airflow
    responsibly.

gleonard-m
gleonard-m reviewed Aug 20, 2025
View reviewed changes
mashalifshin
mashalifshin commented Aug 21, 2025
View reviewed changes
@mashalifshin mashalifshin force-pushed the ae-782-build-dap-collector-job branch from ba47bdf to 65ef418 Compare August 29, 2025 00:53
@mashalifshin mashalifshin marked this pull request as ready for review August 30, 2025 02:22
@mashalifshin mashalifshin force-pushed the ae-782-build-dap-collector-job branch from 6c6a067 to 50591b2 Compare September 3, 2025 01:13
gleonard-m
gleonard-m reviewed Sep 3, 2025
View reviewed changes
@mashalifshin mashalifshin force-pushed the ae-782-build-dap-collector-job branch from 2f54368 to 63053fc Compare September 12, 2025 01:15
gleonard-m and others added 14 commits September 25, 2025 16:08
@gleonard-m @mashalifshin
Initial version of ETL job
7305d05
@gleonard-m @mashalifshin
Removed deleted project
b69a042
@mashalifshin @gleonard-m
Add Glenda's utils, a dev runbook doc, and rename some args and env vars
3e051a7 
Co-Authored-By: Glenda Leonard <[email protected]>
@mashalifshin
WIP: First pass refactor almost complete
7d21e54
@mashalifshin
Working first-pass refactor
89a4cb5
@mashalifshin
Add helper functions for the main for loops, add config objects, add …
3ad955b 
…docs
@mashalifshin
Read job config from GCS bucket
caee055
@mashalifshin
Tweak error handling, whitespace, and add more comments
4fd78a5
@mashalifshin
Whoops, remove manual testing of Nimbus fetching error handling
c7f3bfd
@mashalifshin
Write job output to file in gcs bucket
cfbe5fe
@mashalifshin
Give log files an extension
f8e1e7b
@mashalifshin
Tweak log message
ee4e345
@mashalifshin
WIP tests and add some hopefully helpful type hints
6133976
@mashalifshin
Add dap collection helper tests and fix bug with bucket indexing
e9350e0
gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/README.md Outdated
Comment on lines 17 to 18
Some examples of existing metrics are
- "url visit counting", which increments counters in DAP when a firefox client visits an ad landing page.
Copy link

Choose a reason for hiding this comment

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

An example of the type of metric is not needed here.

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/README.md Outdated
Comment on lines 20 to 22
Great care is taken to preserve privacy and anonymity of these metrics. The DAP technology itself agreggates counts
in separate systems and adds noise. The DAP telemetry feature will only submit a count to DAP once per week per client.
All DAP reports are deleted after 2 weeks.
Copy link

Choose a reason for hiding this comment

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

Details of DAP usage are not needed either since they are not relevant to this ETL job.

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/README.md Outdated
Comment on lines 81 to 83
There is also a `dev_runbook.md` doc that walks through what is required to set up a DAP account, create some DAP
tasks for testing, and the DAP credentials setup and management. The `public_key_to_hpke_config.py` utility will help
with encoding the DAP credentials for consumption by this job.
Copy link

Choose a reason for hiding this comment

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

Remove DAP specific comments

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/ads_incrementality_dap_collector/helpers.py Outdated
return tasks_to_collect


# TODO Trigger Airflow errors
Copy link

Choose a reason for hiding this comment

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

Is this TODO still relevant?

Copy link
Author

Choose a reason for hiding this comment

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

No, this is good now. If we except from this function, it'll bubble up to that top level exception handling in main.py

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/dev_runbook.md Outdated
Copy link

Choose a reason for hiding this comment

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

This fill should be removed and the contents moved to an internal page

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/example_config.json Outdated
"nimbus": {
"api_url": "https://stage.experimenter.nonprod.webservices.mozgcp.net/api/v6/experiments",
"experiments": [{
"slug": "traffic-impact-study-1"
Copy link

Choose a reason for hiding this comment

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

Rename the test experiment slugs to be more generic e.g. exp-1.

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/example_config.json Outdated
"hpke_config": "AQAgAAEAAQAgpdceoGiuWvIiogA8SPCdprkhWMNtLq_y0GSePI7EhXE"
},
"nimbus": {
"api_url": "https://stage.experimenter.nonprod.webservices.mozgcp.net/api/v6/experiments",
Copy link

Choose a reason for hiding this comment

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

A dummy URL can be used here.

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/example_config.json Outdated
"table": "incrementality"
},
"dap": {
"hpke_config": "AQAgAAEAAQAgpdceoGiuWvIiogA8SPCdprkhWMNtLq_y0GSePI7EhXE"
Copy link

Choose a reason for hiding this comment

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

Can an obvious dummy value be used here?

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/public_key_to_hpke_config.py Outdated
Copy link

Choose a reason for hiding this comment

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

This file isn't needed for the job and should be removed.

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/tests/test_mocks.py
"--task-id",
"mubArkO3So8Co1X98CBo62-lSCM4tB-NZPOUGJ83N1o",
"--leader",
"https://dap-09-3.api.divviup.org",
Copy link

Choose a reason for hiding this comment

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

use a dummy endpoint

gleonard-m
gleonard-m reviewed Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/ads_incrementality_dap_collector/constants.py
],
),
bigquery.SchemaField(
"created_at",
Copy link

Choose a reason for hiding this comment

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

Update to created_timestamp ?

mashalifshin
mashalifshin commented Oct 3, 2025
View reviewed changes
jobs/ads-incrementality-dap-collector/ads_incrementality_dap_collector/main.py

@click.command()
@click.option("--gcp_project", help="GCP project id", required=True)
@click.option(
Copy link
Author

Choose a reason for hiding this comment

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

Additional comment from @gleonard-m:
Config file and BQ results table will be in different projects.

This gcp_project should just be used for the location of the config bucket. Rename to be more specific.

And we need to be able to separately specify the gcp_project for the BQ table where the results will be written. That will go in the BQ part of the config json file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gleonard-m gleonard-m gleonard-m left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mashalifshin @gleonard-m