[MAJC-123] Disambiguate invalid_url_error by short circuiting callback code paths for fallback ads by mashalifshin · Pull Request #34 · mozilla-services/majc

mashalifshin · 2025-05-13T22:31:06Z

References

Problem Statement

Currently, there are a couple situations when we send an invalid_url_error to MARS for our majc telemetry. We send it if there is a problem with:

the "report an ad" url
the click callback url
the impression callback url

This works great except that we also register an invalid_url_error for our fallback ads, which don't have live urls from MARS since they are a static part of the bundle.

This means we can't distinguish real invalid_url_errors from the expected behavior around fallback ads, and our code should be a bit smarter and fail out before we send this error for those ads.

Proposed Changes

Add a helper that identifies MozAdsPlacementWithContent as fallback ads if they have a blob image_url as opposed to a live image url from MARS.
Use the helper to skip observing impressions for fallback ads
Use the helper to short circuit out of the click handler for fallback ads
Use the helper to short circuit out of sending reports for the fallback ads
Explicitly validate the url at the beginning of each of the flows, and consistently send the invalid url error and exit before proceeding

There are a couple ways we could have handled this, definitely open to other ideas. We could still observe fallback ads but fail out before we try to make the network call. But it seemed like there wasn't much point in observing them at all if we can never report on it ... a little more efficient perhaps. So short-circuiting for fallbacks and actual invalid urls is the approach throughout.

Also, I chose not to define a first-order property on MozAdsPlacementWithContent and rely on the fact that fallback ads are going to have blob image urls. Seemed like more of an internal detail and I don't want to clutter up the already beefy type.

Open to reviewer thoughts on different approaches.

Verification Steps

A step by step guide of how the PR reviewer can verify that changes are working correctly.

Check out the main branch
In the browser, go into Developer Settings -> Network -> Request blocking and block all requests to https://ads.allizom.org/v1/ads
Open the console
Load the example Article page
Notice the error in the console, you should see an error when the impression observer tries to register an impression and can't do so without the impression callback url
Try to report an ad, go through the dropdown and hit submit. Notice the error in the console.
Check out this branch
Load the example Article page
Verify that there is no longer an error in the console for fallback ads impressions, nor for fallback ads report submissions (the click callback handling is a bit harder to check since you are navigated away, but you can do so in Chrome dev tools if you retain the logs)

luc-lisi

Requesting on change to also include this no-op on fallback clicks as I think that might also be a source of some of our noisy logs, but otherwise looks awesome! Thank you for cleaning this up 🎉

luc-lisi · 2025-05-15T22:17:28Z

packages/core/src/impressions.ts

  public observe(placement: MozAdsPlacementWithContent) {
+    // No need to observe fallback ads -- they are meant for an offline experience so they don't
+    // have dynamic impression urls from MARS, so we can't register an impression for them.
+    if (isFallback(placement)) return


I think we should also include this no-op in clicks.ts when a recordClick occurs.

The approach is great though!

Oh great point, thank you. I will make that update

Okay @luc-lisi I am finally resurfacing from this and it's ready for another look! We now exit early for fallback ads, before sending impressions, clicks, and reports (great catch @cyptm!) to MARS, and we no longer send any invalid url errors for fallbacks on any of those code paths.

cyptm · 2025-05-16T20:51:51Z

@mashalifshin I don't have a ton of context into this area of the code but I've been trying to follow the verification steps outlined. I did have one question around distinguishing what the valid scenarios for which we send the invalid_url_error. If I understand correctly all, is sending it if there is an error with any of the URLs below expected or unexpected?

report an ad url, click callback url, mpression callback url

mashalifshin · 2025-05-20T23:02:09Z

packages/core/src/clicks.ts

+      placementId: placement.placementId,
+    })
+    return
+  }


This un-nests the if-else. Before, we had the invalid_url_error handled in the else, now we check for fallback, then url validity, up front and error and exit before getting to the happy path.

mashalifshin · 2025-05-20T23:03:41Z

packages/core/src/impressions.ts

-      logger.info(`Impression occurred for placement: ${placementId}`, {
-        type: 'impressionObserver.recordImpression.viewed',
-        placementId: placementId,
+    if (!impressionUrl || !URL.canParse(impressionUrl)) {


Same pattern as for recordClick, this un-nests the if-else (where the invalid_url_error used to live on the else branch) and instead we'll first check for fallback, then url validity, and if we don't error and exit, proceed along the happy path.

mashalifshin · 2025-05-20T23:04:29Z

packages/core/src/types.ts

  'recordClick.success' |
  'recordClick.callbackResponseError' |
-  'recordClick.callbackNotFoundError' |
+  'recordClick.invalidCallbackError' |


Making this a bit more general now, and should now only be fired if we get to the time of recording the click or impression and find that the URL is not valid.

mashalifshin · 2025-05-20T23:07:29Z

packages/core/test/clicks.test.ts

-          click: 'http://example.com/click',
-          impression: 'http://example.com/impression',
-          report: 'http://example.com/report',
+  describe('recordClick', () => {


I did some refactoring in this test to unit-test-ify it more. It now just spies on the logger object and makes sure the expected logging functions are called, as opposed to "reaching into" the implementation of the logger and checking that it used the fetch/console log properly, which makes these tests a lot easier to work with.

And then in addition to this, I think there should be a logger test that verifies the logger emits the expected requests, and logs the expected things to the console, but I didn't write that as part of this PR.

Maybe we can tackle in a subsequent PR, I need to understand the expected behavior/how the logger is configured to do that.

Had a conversation with Luc and we actually do have most of those tests already, in instrument.test.ts. So the scope of a new test here is really small, just making sure the toggle for emitting logs is working, so I added that in as part of this PR

mashalifshin · 2025-05-20T23:08:48Z

packages/core/test/display.test.ts

-          click: 'http://example.com/click',
-          impression: 'http://example.com/impression',
-          report: 'http://example.com/report',
+          click: 'https://example.com/click',


No substantial changes in this file, just doing http -> https find-replace for consistency/sanity.

No longer true, we now have some new tests for the reporting flow down at the bottom

mashalifshin · 2025-05-20T23:10:27Z

packages/core/test/logger.test.ts

 const TEST_DEBUG_MESSAGE = 'Test debug message'

-describe('core/logger.ts', () => {
+describe('core/logger.ts DefaultLogger', () => {


Also just renames in this file Mock -> Spy for clarity. This is where I would have started the emitLogs testing but I backed off when I realized I was going too far down the rabbit hole.

emitLogs testing is already in instrument.test.ts actually, hurray 🙌

mashalifshin · 2025-05-20T23:10:54Z

packages/iife/test/display.test.ts

@@ -77,11 +77,11 @@ describe('iife/display.ts', () => {
            format: 'billboard',
            url: 'https://getpocket.com/',
            callbacks: {


Just http -> https changes in this file

mashalifshin · 2025-05-20T23:11:20Z

packages/react/test/components/MozAdsPlacement.test.tsx

@@ -38,11 +38,11 @@ describe('react/components/MozAdsPlacement.tsx', () => {
          format: 'billboard',


Just http -> https changes in this file as well

mashalifshin · 2025-05-20T23:11:48Z

packages/react/test/hooks/useMozAdsPlacement.test.ts

@@ -35,11 +35,11 @@ describe('react/hooks/useMozAdsPlacement.ts', () => {
            format: 'billboard',


And one more, just http -> https changes in this file

mashalifshin · 2025-05-20T23:19:58Z

@mashalifshin I don't have a ton of context into this area of the code but I've been trying to follow the verification steps outlined. I did have one question around distinguishing what the valid scenarios for which we send the invalid_url_error. If I understand correctly all, is sending it if there is an error with any of the URLs below expected or unexpected?

report an ad url, click callback url, mpression callback url

@cyptm Thanks for bearing with my slow replying! Have been figuring out the tests here ...

Yes, you have totally understood it correctly. After this change, we should only be sending the invalid url error if any one of those callback urls are invalid.

So this change special cases the fallback ads, which are static and don't come from MARS and have empty callback urls, so now those should no longer be sending the invalid_url_error each time they are shown or clicked. And we now explicitly, up-front validate the urls for clicks and impressions, and send this error in that specifc case.

The reporting an ad flow didn't require changes for this, since it already attempts to validate the url explicitly by constructing it, and will catch if that constructor throws and then send the invalid_url_error

Edited to Add: Oh actually, now that you mention it, I don't think we're skipping fallback ads in the reporting flow!! Thank you so for mentioning that! Will push up another round of changes soon

…d urls before proceeding with callback fetch calls

…cases

…tp->https in examples

…an event label

luc-lisi

Made a quick comment about the onReport callback not being called during the fallback no-op but I don't think it's being used actively on Pocket, so it is non-blocking.

Otherwise this is awesome! Thank you for all this cleanup :)

luc-lisi · 2025-05-21T19:40:46Z

packages/core/src/clicks.ts

+      placementId: placement.placementId,
+    })
+    return
+  }


luc-lisi · 2025-05-21T19:59:54Z

packages/core/src/display.ts


                const reason = reportSelect.value
-
+                if (isFallback(placement)) return


One thing to note is that by adding this no-op, we will not call the onReport callback passed by a user of the SDK.

I'm not entirely sure if is what we want for fallbacks though as the user might have behavior they always want to see after a report is sent from the frontend?

Oh great callout! Is there any reason not to pull the onReport call up to right before we do the isFallback check? It's really meant only for the call to the reporting URL, not to stop the builder's callback from firing...

Looking at this code more closely, I also see some more error disambiguation that could be good here, like explicitly checking the report url for validity. And we could have different errors for when our call to the report URL fails versus when the builder's onReport callback throws ... I think I'm going to take one more iteration here before merge if that's okay

Yeah I don't see why not to move it up!

Okay, one more round of revisions is ready for 👀

So I actually prioritize our internal report sending work before the builder's callback, but otherwise I did essentially as described above, and added some tests.

…mbiguate errors more

luc-lisi

I cant remember if you were still waiting on re-approval but this is approved!

luc-lisi · 2025-05-28T16:05:17Z

Thanks for all these update 😄

…k code paths for fallback ads (mozilla-services#34) * Don't observe fallback ads * Add isFallback guard for recording clicks, and first check for invalid urls before proceeding with callback fetch calls * Refactor click.test and add a new test for fallback early exit * WIP Logger test that verifies fetch behavior * Update impression observer tests for new behavior, and organize test cases * Organize click test a bit more * Properly mock URL functions, add some happy path impression tests, http->https in examples * Exit early and dont send report for fallback ads as well * Add a lil test to make sure we only emit logs to MARS when they have an event label * In reporting flow, call the builders callback for fallbacks, and disambiguate errors more

mashalifshin requested a review from a team as a code owner May 13, 2025 22:31

mashalifshin force-pushed the majc-123-diambiguate-invalid-url-error branch from 0c04aed to 1bf5409 Compare May 14, 2025 00:44

cyptm self-assigned this May 14, 2025

cyptm self-requested a review May 14, 2025 16:54

luc-lisi requested changes May 15, 2025

View reviewed changes

mashalifshin commented May 20, 2025

View reviewed changes

mashalifshin changed the title ~~[MAJC-123] Disambiguate invalid_url_error by not observing fallback ads~~ [MAJC-123] Disambiguate invalid_url_error by exiting early before registering clicks, impressions, reports May 21, 2025

mashalifshin changed the title ~~[MAJC-123] Disambiguate invalid_url_error by exiting early before registering clicks, impressions, reports~~ [MAJC-123] Disambiguate invalid_url_error by exiting early for fallback ads in the impression, click, and report flows May 21, 2025

mashalifshin changed the title ~~[MAJC-123] Disambiguate invalid_url_error by exiting early for fallback ads in the impression, click, and report flows~~ [MAJC-123] Disambiguate invalid_url_error by exiting callback handling code paths early for fallback ads May 21, 2025

mashalifshin changed the title ~~[MAJC-123] Disambiguate invalid_url_error by exiting callback handling code paths early for fallback ads~~ [MAJC-123] Disambiguate invalid_url_error by exiting callback code paths early for fallback ads May 21, 2025

mashalifshin changed the title ~~[MAJC-123] Disambiguate invalid_url_error by exiting callback code paths early for fallback ads~~ [MAJC-123] Disambiguate invalid_url_error by short circuiting callback code paths for fallback ads May 21, 2025

mashalifshin added 8 commits May 21, 2025 09:21

Don't observe fallback ads

51ca79d

Add isFallback guard for recording clicks, and first check for invali…

db305ac

…d urls before proceeding with callback fetch calls

Refactor click.test and add a new test for fallback early exit

8e225cb

WIP Logger test that verifies fetch behavior

c1eba18

Update impression observer tests for new behavior, and organize test …

724b3de

…cases

Organize click test a bit more

ab7e3c4

Properly mock URL functions, add some happy path impression tests, ht…

9aa719c

…tp->https in examples

Exit early and dont send report for fallback ads as well

5fb174d

mashalifshin force-pushed the majc-123-diambiguate-invalid-url-error branch from 9f077e9 to 5fb174d Compare May 21, 2025 16:21

Add a lil test to make sure we only emit logs to MARS when they have …

cbf77fb

…an event label

luc-lisi approved these changes May 21, 2025

View reviewed changes

In reporting flow, call the builders callback for fallbacks, and disa…

5d04bf7

…mbiguate errors more

cyptm approved these changes May 22, 2025

View reviewed changes

luc-lisi approved these changes May 28, 2025

View reviewed changes

mashalifshin merged commit 191e4f8 into main May 28, 2025
8 checks passed

mashalifshin deleted the majc-123-diambiguate-invalid-url-error branch May 28, 2025 21:50

		@@ -38,11 +38,11 @@ describe('react/components/MozAdsPlacement.tsx', () => {
		format: 'billboard',

		@@ -35,11 +35,11 @@ describe('react/hooks/useMozAdsPlacement.ts', () => {
		format: 'billboard',


		const reason = reportSelect.value

		if (isFallback(placement)) return

Conversation

mashalifshin commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Problem Statement

Proposed Changes

Verification Steps

Uh oh!

luc-lisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyptm commented May 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mashalifshin commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luc-lisi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luc-lisi left a comment

Choose a reason for hiding this comment

Uh oh!

luc-lisi commented May 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mashalifshin commented May 13, 2025 •

edited

Loading

mashalifshin commented May 20, 2025 •

edited

Loading