Closed relud closed 11 months ago
Attention: 21 lines
in your changes are missing coverage. Please review.
Comparison is base (
7dabc47
) 85.41% compared to head (a948bee
) 85.24%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
flagging @chelseatroy and @ncloudioj for additional 👀
@whd fyi: merging this now because validation testing showed that it has no impact without config changes. i will be filing a dsre ticket to request the config changes before the change freeze, in sync with merging a related bigquery-etl pr and a communication to downstream consumers that internal changes are occurring.
merging this now because validation testing showed that it has no impact without config changes
This has been deployed to prod in case you want to triple check it's a no-op without config changes.
for verification three batch jobs have been run against a sample of prod data for a full day with
reportingEnabaled=false
andlogReportingUrls=true
. The first was without this code change and only read contextual_services messages. The second included this code change and also only read contextual_services messages. The third included this code change, read contextual_services and firefox_desktop messages, and hadlimitLegacyDesktopVersion=true
.By comparing the first two jobs verified that this change will have no impact until the new doctypes are sent to it and we update the configuration. This means that this PR is safe to merge, and publishing the new doctypes to the pubsub topic is safe, and once both of those are done we can deploy the configuration changes to switch to the new logic.
By comparing the second and third jobs' number of SendRequest "errors" and extracting aggregate impression counts, I was able to validate the impact of this change aligns with our expectations of slight increase (~1.3%) due to better delivery guarantees in firefox_desktop than contextual_services.
Other common errors were within expected limits of ~1.3% or less change. For less common errors:
RejectedMessageException: Firefox version does not match doctype
went away as expected because that's the intended impact oflimitLegacyDesktopVersion=true
.InvalidUrlException: Could not parse reporting URL
saw a 28% increase from ~9k to ~11.5k, which is big relative to eachother, but small overall.One noteworthy change from validation is that
RejectedMessageException: Matches heuristic from CONSVC-1764
applies acrossfirefox_desktop.top_sites
where before it only applied tocontextual_services.topsites_click
before, and this resulted in a tenfold increase in occurence (from ~2k to ~20k). Inspecting this more closely I found that the number of impacted top sites clicks increased by 1.4% while the rest is attributed to newly rejecting top sites impressions.The other noteworthy change from validation is that I saw significant performance degredation from restricting
user_agent_version
inVerifyMetadata
, sometimes causing pipeline failures. As such, I moved this action intoFilterByDocType
where the messages can be dropped entirely instead of sent to the error output, which resolved the issue.