-
Notifications
You must be signed in to change notification settings - Fork 0
[AE-1142] Include ad client dma in the interaction and request-stats pings #9
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: main
Are you sure you want to change the base?
Conversation
16fb96b to
d5a6075
Compare
gleonard-m
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.
Was a data collection review created for this?
No I didn't do a data review for this change, is it needed? My thought was we're already collecting this data in other pings, and in some cases in other fields of the same ping, but I'm happy to kick that off now if necessary |
| - interaction | ||
| - provider-request-stats | ||
| - request-stats |
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 don't see dma code present on interaction or request-stats yet. One thing that may be annoying about glean is that the fields in the table get named based on the groupings in the schema, so this would make the interactions table have field ad.country_code, ad.region_code, and ad_client.dma. maybe not a real issue since the existing fields are already an arbitrary seeming mixture of prefixes https://dictionary.telemetry.mozilla.org/apps/ads_backend/pings/interaction?page=1
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.
Yeah I think this is okay, it's not perfectly consistent but it's pretty good:
For Interaction and RequestStats, the geo info is under the Ad category, and for ProviderRequestStats its under AdClient category.
I think it's clear enough where to find the geo info.
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.
My suggestion would be to keep single metrics, don't add new metrics which will contain existing data since table columns cannot be deleted. A future update, in co-ordination with DS, could be to add a custom view definition for the ping to group the metrics consistently but any ETL based on these pings would extract the fields into new tables anyway so that update probably not even worth it.
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 agree with Glenda on reusing the existing dma metric and including that in the new pings, sorry if my previous comment misled. then this LGTM
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.
Oh I see, yes I had misunderstood initially, and thank you both for jumping in and clarifying! I have updated it once more. Now, optimizing for reusing existing metrics for the same data, we have:
Interaction, RequestStats
ad.country_codead.region_codead_client.dma_code
ProviderRequestStats
ad_client.country_codead_client.region_codead_client.dma_code
(so unfortunately I already have some existing duplication for country_code and region_code from when I put in ProviderRequestStats, but at least for the dma we can reuse)
d8d396d to
c5a22e2
Compare
|
DATA REVIEW REQUEST
In preparation for fully rolling out OHTTP between Desktop FFox and MARS, we need to capture some coarse geo info about the ad client in all our pings, since we will no longer be able to rely on the client's IP address to derive this data. We were already capturing the CountryCode and the RegionCode in interaction and request-stats, this change adds capturing the DMA. (provider-request-stats already captures all three geo info fields).
When we make requests to ad providers, we need to let the ad server know roughly what area the client is coming from so that only relevant ads are returned. So in the case of the request-stats ping, we record for system observability and correctness. In the case of the interaction ping, we record for those same reasons, and for our ad business reporting internally.
With the introduction of OHTTP between FFox Desktop and MARS, we no longer have the X-Forwarded-For ip_address field that we used to have in the standard glean ingestion. We are replacing that with three explicit geo metrics, CountryCode, RegionCode, and DMACode. This change completes the third geo metric. (This answer is based on some preliminary conversations in #glean, summarized in https://mozilla-hub.atlassian.net/browse/AE-1142)
Soon it won't be able to, so this change shores up our preparation for OHTTP across the metrics we collect.
This collection is Glean so is documented in the Glean Dictionary.
This collection will be collected permanently.
All channels, countries, and locales. No filters.
These collections are Glean. The opt-out can be found in the product's preferences.
This data will be part of standard ad reporting for observability, correctness of ad targeting, and as internal business metrics to understand the coarse geo info of where ads are being requested from and returned.
This data will surface in standard dashboards for ad interactions (impression and click data tables), and MozAds team will use this to understand revenue/business insights.
No. |
Tickets or other relevant links:
https://mozilla-hub.atlassian.net/browse/AE-1142
What's Here
In preparation for rolling out OHTTP between Desktop FFox and MARS, we need to capture all the ad client's geo fields in all our pings -- specifically the interaction and request-stats ping.
Things I'd Like Feedback On
Lmk if you see any issues with capturing this. Looks like it is already available in the backend.
Testing
This can be tested via the companion PR in MARS that use the updated pings structure, see the Testing section there.
Any concerns with backwards compatibility?
No, these changes are additive.